[prev in list] [next in list] [prev in thread] [next in thread] 

List:       mercurial
Subject:    Re: [PATCH] Use opts.get(key) instead of opts[key] everywhere it
From:       "Alexis S. L. Carvalho" <alexis () cecm ! usp ! br>
Date:       2006-07-31 22:48:37
Message-ID: 20060731224836.GA11629 () cecm ! usp ! br
[Download RAW message or body]

Thus spake Brendan Cully:
> # HG changeset patch
> # User Brendan Cully <brendan@kublai.com>
> # Node ID 6104b22f522e2578f3a648ac25b14282a9332460
> # Parent  a31f0f2997e90d95db7d089551568a4a7a1ff653
> Use opts.get(key) instead of opts[key] everywhere it might be unset

I haven't read the whole patch but there are a few problems:

- sometimes the code expects a list (or a string), not None:

> @@ -62,8 +62,8 @@ def matchpats(repo, pats=[], opts={}, he
>  def matchpats(repo, pats=[], opts={}, head=''):
>      cwd = repo.getcwd()
>      if not pats and cwd:
> -        opts['include'] = [os.path.join(cwd, i) for i in opts['include']]
> -        opts['exclude'] = [os.path.join(cwd, x) for x in opts['exclude']]
> +        opts['include'] = [os.path.join(cwd, i) for i in opts.get('include')]
> +        opts['exclude'] = [os.path.join(cwd, x) for x in opts.get('exclude')]

  e.g. if there's no key named "include", this would raise a "TypeError:
  iteration over non-sequence"

> @@ -872,14 +872,14 @@ def archive(ui, repo, dest, **opts):
>          raise util.Abort(_('repository root cannot be destination'))
>      dummy, matchfn, dummy = matchpats(repo, [], opts)
>      kind = opts.get('type') or 'files'
> -    prefix = opts['prefix']
> +    prefix = opts.get('prefix')
>      if dest == '-':
>          if kind == 'files':
>              raise util.Abort(_('cannot archive plain files to stdout'))
>          dest = sys.stdout
>          if not prefix: prefix = os.path.basename(repo.root) + '-%h'
>      prefix = make_filename(repo, prefix, node)

  and here make_filename expects the second argument to be a string


- default values are not taken into account:

> @@ -1736,11 +1736,11 @@ def import_(ui, repo, patch1, *patches, 
>      """
>      patches = (patch1,) + patches
>  
> -    if not opts['force']:
> +    if not opts.get('force'):
>          bail_if_changed(repo)
>  
> -    d = opts["base"]
> -    strip = opts["strip"]
> +    d = opts.get("base")
> +    strip = opts.get("strip")

  strip should default to 1 if it's not explicitly set.

  (but this was the only default value different from None/[]/'' that I
  noticed after a *very* quick scan of the command table)

  OTOH specifying the default value in two places (in the command table
  and in the code) is not exactly elegant...

In most cases, hg is only interested in a boolean value, so most of the
patch is probably ok, but there's still some work to be done.

Alexis
_______________________________________________
Mercurial mailing list
Mercurial@selenic.com
http://selenic.com/mailman/listinfo/mercurial
[prev in list] [next in list] [prev in thread] [next in thread]