Patchwork [1,of,3] revset: extract a helper to parse integer range

login
register
mail settings
Submitter Yuya Nishihara
Date Jan. 31, 2019, 2:19 p.m.
Message ID <5e5dda247ee2ba8407bc.1548944358@mimosa>
Download mbox | patch
Permalink /patch/38254/
State Superseded
Headers show

Comments

Yuya Nishihara - Jan. 31, 2019, 2:19 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1548562733 -32400
#      Sun Jan 27 13:18:53 2019 +0900
# Node ID 5e5dda247ee2ba8407bc4932118a944c1959b1f9
# Parent  1aa52287973e180eaf23c32dd454d38537facb8e
revset: extract a helper to parse integer range

It's getting common. As a first step, this patch adds getintrange() and
makes followlines() use it.
Anton Shestakov - Jan. 31, 2019, 3:21 p.m.
On Thu, 31 Jan 2019 23:19:18 +0900
Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1548562733 -32400
> #      Sun Jan 27 13:18:53 2019 +0900
> # Node ID 5e5dda247ee2ba8407bc4932118a944c1959b1f9
> # Parent  1aa52287973e180eaf23c32dd454d38537facb8e
> revset: extract a helper to parse integer range
> 
> It's getting common. As a first step, this patch adds getintrange() and
> makes followlines() use it.
> 
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -44,6 +44,7 @@ getinteger = revsetlang.getinteger
>  getboolean = revsetlang.getboolean
>  getlist = revsetlang.getlist
>  getrange = revsetlang.getrange
> +getintrange = revsetlang.getintrange
>  getargs = revsetlang.getargs
>  getargsdict = revsetlang.getargsdict
>  
> @@ -1067,11 +1068,10 @@ def followlines(repo, subset, x):
>      # i18n: "followlines" is a keyword
>      msg = _("followlines expects exactly one file")
>      fname = scmutil.parsefollowlinespattern(repo, rev, pat, msg)
> -    # i18n: "followlines" is a keyword
> -    lr = getrange(args['lines'][0], _("followlines expects a line range"))
> -    fromline, toline = [getinteger(a, _("line range bounds must be integers"))
> -                        for a in lr]
> -    fromline, toline = util.processlinerange(fromline, toline)
> +    fromline, toline = util.processlinerange(
> +        *getintrange(args['lines'][0],
> +                     # i18n: "followlines" is a keyword
> +                     _("followlines expects an integer line range")))

Maybe it's me, but I couldn't parse "integer line range" without taking
a moment to think. I think the two old messages are easier to
understand, because one is a broad description of what's expected and
the other is an error message just in case. I mean, it's already logical
that a range of lines would be limited by integers (and not by floats,
for example). I'd say something like "followlines expects a line number
or a range" (considering the 2nd patch) has good balance between
simplicity and precision, and if user decides to use 3.14 as a line
number or a range bound, then getintrange() should complain at that
specifically.

> diff --git a/mercurial/revsetlang.py b/mercurial/revsetlang.py
> --- a/mercurial/revsetlang.py
> +++ b/mercurial/revsetlang.py
> @@ -240,6 +240,15 @@ def getrange(x, err):
>          return None, None
>      raise error.ParseError(err)
>  
> +def getintrange(x, err, deffirst=_notset, deflast=_notset):
> +    """Get [first, last] integer range (both inclusive) from a parsed tree
> +
> +    If any of the sides omitted, and if no default provided, ParseError will
> +    be raised.
> +    """
> +    a, b = getrange(x, err)
> +    return getinteger(a, err, deffirst), getinteger(b, err, deflast)
> +
>  def getargs(x, min, max, err):
>      l = getlist(x)
>      if len(l) < min or (max >= 0 and len(l) > max):
> diff --git a/tests/test-annotate.t b/tests/test-annotate.t
> --- a/tests/test-annotate.t
> +++ b/tests/test-annotate.t
> @@ -818,7 +818,7 @@ check error cases
>    hg: parse error: followlines requires a line range
>    [255]
>    $ hg log -r 'followlines(baz, 1)'
> -  hg: parse error: followlines expects a line range
> +  hg: parse error: followlines expects an integer line range
>    [255]
>    $ hg log -r 'followlines(baz, 1:2, startrev=desc("b"))'
>    hg: parse error: followlines expects exactly one revision
> @@ -827,13 +827,13 @@ check error cases
>    hg: parse error: followlines expects exactly one file
>    [255]
>    $ hg log -r 'followlines(baz, 1:)'
> -  hg: parse error: line range bounds must be integers
> +  hg: parse error: followlines expects an integer line range
>    [255]
>    $ hg log -r 'followlines(baz, :1)'
> -  hg: parse error: line range bounds must be integers
> +  hg: parse error: followlines expects an integer line range
>    [255]
>    $ hg log -r 'followlines(baz, x:4)'
> -  hg: parse error: line range bounds must be integers
> +  hg: parse error: followlines expects an integer line range
>    [255]
>    $ hg log -r 'followlines(baz, 5:4)'
>    hg: parse error: line range must be positive
Yuya Nishihara - Jan. 31, 2019, 10:51 p.m.
On Thu, 31 Jan 2019 23:21:48 +0800, Anton Shestakov wrote:
> On Thu, 31 Jan 2019 23:19:18 +0900
> Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1548562733 -32400
> > #      Sun Jan 27 13:18:53 2019 +0900
> > # Node ID 5e5dda247ee2ba8407bc4932118a944c1959b1f9
> > # Parent  1aa52287973e180eaf23c32dd454d38537facb8e
> > revset: extract a helper to parse integer range
> > 
> > It's getting common. As a first step, this patch adds getintrange() and
> > makes followlines() use it.
> > 
> > diff --git a/mercurial/revset.py b/mercurial/revset.py
> > --- a/mercurial/revset.py
> > +++ b/mercurial/revset.py
> > @@ -44,6 +44,7 @@ getinteger = revsetlang.getinteger
> >  getboolean = revsetlang.getboolean
> >  getlist = revsetlang.getlist
> >  getrange = revsetlang.getrange
> > +getintrange = revsetlang.getintrange
> >  getargs = revsetlang.getargs
> >  getargsdict = revsetlang.getargsdict
> >  
> > @@ -1067,11 +1068,10 @@ def followlines(repo, subset, x):
> >      # i18n: "followlines" is a keyword
> >      msg = _("followlines expects exactly one file")
> >      fname = scmutil.parsefollowlinespattern(repo, rev, pat, msg)
> > -    # i18n: "followlines" is a keyword
> > -    lr = getrange(args['lines'][0], _("followlines expects a line range"))
> > -    fromline, toline = [getinteger(a, _("line range bounds must be integers"))
> > -                        for a in lr]
> > -    fromline, toline = util.processlinerange(fromline, toline)
> > +    fromline, toline = util.processlinerange(
> > +        *getintrange(args['lines'][0],
> > +                     # i18n: "followlines" is a keyword
> > +                     _("followlines expects an integer line range")))
> 
> Maybe it's me, but I couldn't parse "integer line range" without taking
> a moment to think. I think the two old messages are easier to
> understand, because one is a broad description of what's expected and
> the other is an error message just in case. I mean, it's already logical
> that a range of lines would be limited by integers (and not by floats,
> for example). I'd say something like "followlines expects a line number
> or a range" (considering the 2nd patch) has good balance between
> simplicity and precision, and if user decides to use 3.14 as a line
> number or a range bound, then getintrange() should complain at that
> specifically.

Makes sense. I just wanted to simplify the code in this patch. I'll update
the error messages and resend.

Patch

diff --git a/mercurial/revset.py b/mercurial/revset.py
--- a/mercurial/revset.py
+++ b/mercurial/revset.py
@@ -44,6 +44,7 @@  getinteger = revsetlang.getinteger
 getboolean = revsetlang.getboolean
 getlist = revsetlang.getlist
 getrange = revsetlang.getrange
+getintrange = revsetlang.getintrange
 getargs = revsetlang.getargs
 getargsdict = revsetlang.getargsdict
 
@@ -1067,11 +1068,10 @@  def followlines(repo, subset, x):
     # i18n: "followlines" is a keyword
     msg = _("followlines expects exactly one file")
     fname = scmutil.parsefollowlinespattern(repo, rev, pat, msg)
-    # i18n: "followlines" is a keyword
-    lr = getrange(args['lines'][0], _("followlines expects a line range"))
-    fromline, toline = [getinteger(a, _("line range bounds must be integers"))
-                        for a in lr]
-    fromline, toline = util.processlinerange(fromline, toline)
+    fromline, toline = util.processlinerange(
+        *getintrange(args['lines'][0],
+                     # i18n: "followlines" is a keyword
+                     _("followlines expects an integer line range")))
 
     fctx = repo[rev].filectx(fname)
     descend = False
diff --git a/mercurial/revsetlang.py b/mercurial/revsetlang.py
--- a/mercurial/revsetlang.py
+++ b/mercurial/revsetlang.py
@@ -240,6 +240,15 @@  def getrange(x, err):
         return None, None
     raise error.ParseError(err)
 
+def getintrange(x, err, deffirst=_notset, deflast=_notset):
+    """Get [first, last] integer range (both inclusive) from a parsed tree
+
+    If any of the sides omitted, and if no default provided, ParseError will
+    be raised.
+    """
+    a, b = getrange(x, err)
+    return getinteger(a, err, deffirst), getinteger(b, err, deflast)
+
 def getargs(x, min, max, err):
     l = getlist(x)
     if len(l) < min or (max >= 0 and len(l) > max):
diff --git a/tests/test-annotate.t b/tests/test-annotate.t
--- a/tests/test-annotate.t
+++ b/tests/test-annotate.t
@@ -818,7 +818,7 @@  check error cases
   hg: parse error: followlines requires a line range
   [255]
   $ hg log -r 'followlines(baz, 1)'
-  hg: parse error: followlines expects a line range
+  hg: parse error: followlines expects an integer line range
   [255]
   $ hg log -r 'followlines(baz, 1:2, startrev=desc("b"))'
   hg: parse error: followlines expects exactly one revision
@@ -827,13 +827,13 @@  check error cases
   hg: parse error: followlines expects exactly one file
   [255]
   $ hg log -r 'followlines(baz, 1:)'
-  hg: parse error: line range bounds must be integers
+  hg: parse error: followlines expects an integer line range
   [255]
   $ hg log -r 'followlines(baz, :1)'
-  hg: parse error: line range bounds must be integers
+  hg: parse error: followlines expects an integer line range
   [255]
   $ hg log -r 'followlines(baz, x:4)'
-  hg: parse error: line range bounds must be integers
+  hg: parse error: followlines expects an integer line range
   [255]
   $ hg log -r 'followlines(baz, 5:4)'
   hg: parse error: line range must be positive