Patchwork purge: use opts.get()

login
register
mail settings
Submitter Gregory Szorc
Date May 16, 2016, 9:24 p.m.
Message ID <a2a87f12649b7485f42b.1463433897@gps-windows-desktop.sfo1.mozilla.com>
Download mbox | patch
Permalink /patch/15151/
State Accepted
Headers show

Comments

Gregory Szorc - May 16, 2016, 9:24 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1463433699 25200
#      Mon May 16 14:21:39 2016 -0700
# Node ID a2a87f12649b7485f42b4d4d25e8f27e5a0c3f07
# Parent  90d84e1e427a9d65aedd870cdb7283f84bb30141
purge: use opts.get()

Most commands use opts.get() to retrieve values for options
that may not be explicitly passed. purge wasn't.

This makes it easier to call purge() from 3rd party extensions.
Augie Fackler - May 24, 2016, 3 p.m.
On Mon, May 16, 2016 at 02:24:57PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1463433699 25200
> #      Mon May 16 14:21:39 2016 -0700
> # Node ID a2a87f12649b7485f42b4d4d25e8f27e5a0c3f07
> # Parent  90d84e1e427a9d65aedd870cdb7283f84bb30141
> purge: use opts.get()

Sure, queued. Thanks!

>
> Most commands use opts.get() to retrieve values for options
> that may not be explicitly passed. purge wasn't.
>
> This makes it easier to call purge() from 3rd party extensions.
>
> diff --git a/hgext/purge.py b/hgext/purge.py
> --- a/hgext/purge.py
> +++ b/hgext/purge.py
> @@ -79,44 +79,44 @@ def purge(ui, repo, *dirs, **opts):
>      If directories are given on the command line, only files in these
>      directories are considered.
>
>      Be careful with purge, as you could irreversibly delete some files
>      you forgot to add to the repository. If you only want to print the
>      list of files that this program would delete, use the --print
>      option.
>      '''
> -    act = not opts['print']
> +    act = not opts.get('print')
>      eol = '\n'
> -    if opts['print0']:
> +    if opts.get('print0'):
>          eol = '\0'
>          act = False # --print0 implies --print
> -    removefiles = opts['files']
> -    removedirs = opts['dirs']
> +    removefiles = opts.get('files')
> +    removedirs = opts.get('dirs')
>      if not removefiles and not removedirs:
>          removefiles = True
>          removedirs = True
>
>      def remove(remove_func, name):
>          if act:
>              try:
>                  remove_func(repo.wjoin(name))
>              except OSError:
>                  m = _('%s cannot be removed') % name
> -                if opts['abort_on_err']:
> +                if opts.get('abort_on_err'):
>                      raise error.Abort(m)
>                  ui.warn(_('warning: %s\n') % m)
>          else:
>              ui.write('%s%s' % (name, eol))
>
>      match = scmutil.match(repo[None], dirs, opts)
>      if removedirs:
>          directories = []
>          match.explicitdir = match.traversedir = directories.append
> -    status = repo.status(match=match, ignored=opts['all'], unknown=True)
> +    status = repo.status(match=match, ignored=opts.get('all'), unknown=True)
>
>      if removefiles:
>          for f in sorted(status.unknown + status.ignored):
>              if act:
>                  ui.note(_('removing file %s\n') % f)
>              remove(util.unlink, f)
>
>      if removedirs:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 27, 2016, 2:32 p.m.
This is not the first time we need to do this, should we grow some
semi-official API to call a command? (some subset of dispatch with a
pythonic way of passing argument)

On 05/24/2016 05:00 PM, Augie Fackler wrote:
> On Mon, May 16, 2016 at 02:24:57PM -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1463433699 25200
>> #      Mon May 16 14:21:39 2016 -0700
>> # Node ID a2a87f12649b7485f42b4d4d25e8f27e5a0c3f07
>> # Parent  90d84e1e427a9d65aedd870cdb7283f84bb30141
>> purge: use opts.get()
> 
> Sure, queued. Thanks!
> 
>>
>> Most commands use opts.get() to retrieve values for options
>> that may not be explicitly passed. purge wasn't.
>>
>> This makes it easier to call purge() from 3rd party extensions.
>>
>> diff --git a/hgext/purge.py b/hgext/purge.py
>> --- a/hgext/purge.py
>> +++ b/hgext/purge.py
>> @@ -79,44 +79,44 @@ def purge(ui, repo, *dirs, **opts):
>>      If directories are given on the command line, only files in these
>>      directories are considered.
>>
>>      Be careful with purge, as you could irreversibly delete some files
>>      you forgot to add to the repository. If you only want to print the
>>      list of files that this program would delete, use the --print
>>      option.
>>      '''
>> -    act = not opts['print']
>> +    act = not opts.get('print')
>>      eol = '\n'
>> -    if opts['print0']:
>> +    if opts.get('print0'):
>>          eol = '\0'
>>          act = False # --print0 implies --print
>> -    removefiles = opts['files']
>> -    removedirs = opts['dirs']
>> +    removefiles = opts.get('files')
>> +    removedirs = opts.get('dirs')
>>      if not removefiles and not removedirs:
>>          removefiles = True
>>          removedirs = True
>>
>>      def remove(remove_func, name):
>>          if act:
>>              try:
>>                  remove_func(repo.wjoin(name))
>>              except OSError:
>>                  m = _('%s cannot be removed') % name
>> -                if opts['abort_on_err']:
>> +                if opts.get('abort_on_err'):
>>                      raise error.Abort(m)
>>                  ui.warn(_('warning: %s\n') % m)
>>          else:
>>              ui.write('%s%s' % (name, eol))
>>
>>      match = scmutil.match(repo[None], dirs, opts)
>>      if removedirs:
>>          directories = []
>>          match.explicitdir = match.traversedir = directories.append
>> -    status = repo.status(match=match, ignored=opts['all'], unknown=True)
>> +    status = repo.status(match=match, ignored=opts.get('all'), unknown=True)
>>
>>      if removefiles:
>>          for f in sorted(status.unknown + status.ignored):
>>              if act:
>>                  ui.note(_('removing file %s\n') % f)
>>              remove(util.unlink, f)
>>
>>      if removedirs:
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Gregory Szorc - June 1, 2016, 2:35 a.m.
On Fri, May 27, 2016 at 7:32 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> This is not the first time we need to do this, should we grow some
> semi-official API to call a command? (some subset of dispatch with a
> pythonic way of passing argument)
>

I'm not sure what you have in mind. We do have the argument names from
@command. IIRC dispatch already ensures all CLI arguments have named
arguments in the function call. Perhaps we'd need to split that out and
make an API so extensions can call that? But you can't enforce that since
an extension can get a handle on the command function and call it directly.
I suppose it's better than nothing.


>
> On 05/24/2016 05:00 PM, Augie Fackler wrote:
> > On Mon, May 16, 2016 at 02:24:57PM -0700, Gregory Szorc wrote:
> >> # HG changeset patch
> >> # User Gregory Szorc <gregory.szorc@gmail.com>
> >> # Date 1463433699 25200
> >> #      Mon May 16 14:21:39 2016 -0700
> >> # Node ID a2a87f12649b7485f42b4d4d25e8f27e5a0c3f07
> >> # Parent  90d84e1e427a9d65aedd870cdb7283f84bb30141
> >> purge: use opts.get()
> >
> > Sure, queued. Thanks!
> >
> >>
> >> Most commands use opts.get() to retrieve values for options
> >> that may not be explicitly passed. purge wasn't.
> >>
> >> This makes it easier to call purge() from 3rd party extensions.
> >>
> >> diff --git a/hgext/purge.py b/hgext/purge.py
> >> --- a/hgext/purge.py
> >> +++ b/hgext/purge.py
> >> @@ -79,44 +79,44 @@ def purge(ui, repo, *dirs, **opts):
> >>      If directories are given on the command line, only files in these
> >>      directories are considered.
> >>
> >>      Be careful with purge, as you could irreversibly delete some files
> >>      you forgot to add to the repository. If you only want to print the
> >>      list of files that this program would delete, use the --print
> >>      option.
> >>      '''
> >> -    act = not opts['print']
> >> +    act = not opts.get('print')
> >>      eol = '\n'
> >> -    if opts['print0']:
> >> +    if opts.get('print0'):
> >>          eol = '\0'
> >>          act = False # --print0 implies --print
> >> -    removefiles = opts['files']
> >> -    removedirs = opts['dirs']
> >> +    removefiles = opts.get('files')
> >> +    removedirs = opts.get('dirs')
> >>      if not removefiles and not removedirs:
> >>          removefiles = True
> >>          removedirs = True
> >>
> >>      def remove(remove_func, name):
> >>          if act:
> >>              try:
> >>                  remove_func(repo.wjoin(name))
> >>              except OSError:
> >>                  m = _('%s cannot be removed') % name
> >> -                if opts['abort_on_err']:
> >> +                if opts.get('abort_on_err'):
> >>                      raise error.Abort(m)
> >>                  ui.warn(_('warning: %s\n') % m)
> >>          else:
> >>              ui.write('%s%s' % (name, eol))
> >>
> >>      match = scmutil.match(repo[None], dirs, opts)
> >>      if removedirs:
> >>          directories = []
> >>          match.explicitdir = match.traversedir = directories.append
> >> -    status = repo.status(match=match, ignored=opts['all'],
> unknown=True)
> >> +    status = repo.status(match=match, ignored=opts.get('all'),
> unknown=True)
> >>
> >>      if removefiles:
> >>          for f in sorted(status.unknown + status.ignored):
> >>              if act:
> >>                  ui.note(_('removing file %s\n') % f)
> >>              remove(util.unlink, f)
> >>
> >>      if removedirs:
> >> _______________________________________________
> >> Mercurial-devel mailing list
> >> Mercurial-devel@mercurial-scm.org
> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
Pierre-Yves David - June 7, 2016, 7:15 p.m.
On 06/01/2016 04:35 AM, Gregory Szorc wrote:
> 
> 
> On Fri, May 27, 2016 at 7:32 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
> 
>     This is not the first time we need to do this, should we grow some
>     semi-official API to call a command? (some subset of dispatch with a
>     pythonic way of passing argument)
> 
> 
> I'm not sure what you have in mind. We do have the argument names from
> @command. IIRC dispatch already ensures all CLI arguments have named
> arguments in the function call. Perhaps we'd need to split that out and
> make an API so extensions can call that? But you can't enforce that
> since an extension can get a handle on the command function and call it
> directly. I suppose it's better than nothing.

Yes, splitting dispatch so that one can call something like

  callcommand('commit', 'myfile.txt', message="foobar",
user="babar@savannah")

And still have all the dispatch logic apply (except for alias/default).
Right now, you have to call:

  dispatch('commit', 'myfile.txt', '--message', "foobar", '--user',
"babar@savannah")

This is vastly unpythonic.

Sure, extension could still grab the handle directly, but they would not
have a good reason to do so anymore.

Patch

diff --git a/hgext/purge.py b/hgext/purge.py
--- a/hgext/purge.py
+++ b/hgext/purge.py
@@ -79,44 +79,44 @@  def purge(ui, repo, *dirs, **opts):
     If directories are given on the command line, only files in these
     directories are considered.
 
     Be careful with purge, as you could irreversibly delete some files
     you forgot to add to the repository. If you only want to print the
     list of files that this program would delete, use the --print
     option.
     '''
-    act = not opts['print']
+    act = not opts.get('print')
     eol = '\n'
-    if opts['print0']:
+    if opts.get('print0'):
         eol = '\0'
         act = False # --print0 implies --print
-    removefiles = opts['files']
-    removedirs = opts['dirs']
+    removefiles = opts.get('files')
+    removedirs = opts.get('dirs')
     if not removefiles and not removedirs:
         removefiles = True
         removedirs = True
 
     def remove(remove_func, name):
         if act:
             try:
                 remove_func(repo.wjoin(name))
             except OSError:
                 m = _('%s cannot be removed') % name
-                if opts['abort_on_err']:
+                if opts.get('abort_on_err'):
                     raise error.Abort(m)
                 ui.warn(_('warning: %s\n') % m)
         else:
             ui.write('%s%s' % (name, eol))
 
     match = scmutil.match(repo[None], dirs, opts)
     if removedirs:
         directories = []
         match.explicitdir = match.traversedir = directories.append
-    status = repo.status(match=match, ignored=opts['all'], unknown=True)
+    status = repo.status(match=match, ignored=opts.get('all'), unknown=True)
 
     if removefiles:
         for f in sorted(status.unknown + status.ignored):
             if act:
                 ui.note(_('removing file %s\n') % f)
             remove(util.unlink, f)
 
     if removedirs: