Patchwork [1,of,5] revset: have `min` and `max` method of smartset works when empty

login
register
mail settings
Submitter Pierre-Yves David
Date March 21, 2014, 6:28 p.m.
Message ID <ad232d545933fc511fea.1395426484@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/4019/
State Accepted
Headers show

Comments

Pierre-Yves David - March 21, 2014, 6:28 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1395367234 25200
#      Thu Mar 20 19:00:34 2014 -0700
# Node ID ad232d545933fc511feaeceb85ba43c4cb1d5415
# Parent  170d6d591a7dbc09bfe1b509dfd8f39991e653a9
revset: have `min` and `max` method of smartset works when empty

When empty `baseset` and `lazyset` had crashing `min` and `max` methods. They
now return None when empty. Same as the other smart set do.

This is not easily testable now. But the following changeset use this one and
test it.
Matt Mackall - March 21, 2014, 10:18 p.m.
On Fri, 2014-03-21 at 11:28 -0700, pierre-yves.david@ens-lyon.org wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1395367234 25200
> #      Thu Mar 20 19:00:34 2014 -0700
> # Node ID ad232d545933fc511feaeceb85ba43c4cb1d5415
> # Parent  170d6d591a7dbc09bfe1b509dfd8f39991e653a9
> revset: have `min` and `max` method of smartset works when empty
> 
> When empty `baseset` and `lazyset` had crashing `min` and `max` methods. They
> now return None when empty. Same as the other smart set do.

Why is this correct? I would think we would want it to behave like the
empty list it was replacing:

 >>> min([])
 Traceback (most recent call last):
   File "<stdin>", line 1, in <module>
 ValueError: min() arg is an empty sequence

Instead, we'll get some surprise like this:

 repo[revs.min()] -> working context lolwut
Pierre-Yves David - March 21, 2014, 10:27 p.m.
On 03/21/2014 03:18 PM, Matt Mackall wrote:
> On Fri, 2014-03-21 at 11:28 -0700, pierre-yves.david@ens-lyon.org wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1395367234 25200
>> #      Thu Mar 20 19:00:34 2014 -0700
>> # Node ID ad232d545933fc511feaeceb85ba43c4cb1d5415
>> # Parent  170d6d591a7dbc09bfe1b509dfd8f39991e653a9
>> revset: have `min` and `max` method of smartset works when empty
>>
>> When empty `baseset` and `lazyset` had crashing `min` and `max` methods. They
>> now return None when empty. Same as the other smart set do.
>
> Why is this correct? I would think we would want it to behave like the
> empty list it was replacing:

This is "Correct" in the sense that it is what the implementation of 
other smart set are already doing

>   >>> min([])
>   Traceback (most recent call last):
>     File "<stdin>", line 1, in <module>
>   ValueError: min() arg is an empty sequence
>
> Instead, we'll get some surprise like this:
>
>   repo[revs.min()] -> working context lolwut

I agree the current behavior (we are aligning to) sound bad.

I'll redo this series, moving code toward the raise Value error version.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -2208,14 +2208,18 @@  class baseset(list):
 
         This is part of the mandatory API for smartset."""
         self.sort(reverse=True)
 
     def min(self):
-        return min(self)
+        if self:
+            return min(self)
+        return None
 
     def max(self):
-        return max(self)
+        if self:
+            return max(self)
+        return None
 
     def set(self):
         """Returns a set or a smartset containing all the elements.
 
         The returned structure should be the fastest option for membership
@@ -2323,14 +2327,18 @@  class lazyset(object):
 
     def descending(self):
         self._subset.sort(reverse=True)
 
     def min(self):
-        return min(self)
+        if self:
+            return min(self)
+        return None
 
     def max(self):
-        return max(self)
+        if self:
+            return max(self)
+        return None
 
     def __contains__(self, x):
         c = self._cache
         if x not in c:
             c[x] = x in self._subset and self._condition(x)