| Submitter | Gregory Szorc |
|---|---|
| Date | March 8, 2015, 7:54 p.m. |
| Message ID | <46c14d03f3e05aa9992d.1425844457@vm-ubuntu-main.gateway.sonic.net> |
| Download | mbox | patch |
| Permalink | /patch/7944/ |
| State | Changes Requested |
| Delegated to: | Pierre-Yves David |
| Headers | show |
Comments
On 03/08/2015 12:54 PM, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1423509114 28800 > # Mon Feb 09 11:11:54 2015 -0800 > # Node ID 46c14d03f3e05aa9992d074311c5e92924c2dda3 > # Parent 0f9c6588afcbaaca41b8a5ff17d8969e7f6d157a > ui: change how default is handled by getpath() > > Callers of ui.expandpath perform a pattern like > |ui.expandpath(dest or 'default-push', dest or 'default')|. The > intent of this pattern is somewhat difficult to read and requires > callers contain logic for path fallback. > > Since we now have a proper paths API, we now have the opportunity > to move this logic from the caller into the new API so callers > aren't burdened with it. > > This patch changes getpath() to perform default fallback internally, > rather than burden callers. > > The new code is a bit more complicated than I would like, especially > the new implementation of ui.expandpath. However, this complication > will eventually absorb complication in callers of ui.expandpath and > will making calling code simpler. This will be addressed in subsequent > patches. This patch is scary and the implementation looks like a soup of if/elif/else statement I cannot get my head around. Can we build something simpler or document why this has to be some scary ? > diff --git a/mercurial/ui.py b/mercurial/ui.py > --- a/mercurial/ui.py > +++ b/mercurial/ui.py > @@ -531,9 +531,16 @@ class ui(object): > """Return repository location relative to cwd or from [paths]""" > if util.hasscheme(loc) or os.path.isdir(os.path.join(loc, '.hg')): > return loc > > - p = self.paths.getpath(loc, default=default) > + if loc == 'default-push': > + p = self.paths.getpath(loc, default=True) > + elif loc == 'default': > + p = self.paths.getpath(loc) > + else: > + assert default != 'default-push' > + p = self.paths.getpath(loc, default=default=='default') > + Can we get a single `self.paths.getpath` call. preparing parameter on previous line if necessary ? This will make the conditional clearer (because shorter) and the code easier to modify (less line to touch if getpath changes)) I'm also curious about why this is not a valid version (and probably deserve a comment): p = self.paths.getpath(loc, default=(default in ('default', 'default-push)') > if p: > return p.loc > return loc > > @@ -949,22 +956,34 @@ class paths(dict): > continue > > self[name] = path(name, rawloc=loc) > > - def getpath(self, name, default=None): > + def getpath(self, name, default=False): > """Return a ``path`` for the specified name, falling back to a default. > > - Returns the first of ``name`` or ``default`` that is present, or None > - if neither is present. > + If ``default`` is True, we attempt to resolve the default path. > + If ``default`` is the string ``push``, we attempt to resolve the > + default push path. I've not idea why we have to use such logic here. it seems a bit witchcrafty to me. > + > + Returns ``None`` if a path could not be found. > """ > try: > return self[name] > except KeyError: > - if default is not None: > - try: > - return self[default] > - except KeyError: > - pass > + pass > + > + if default == 'push': > + try: > + return self['default-push'] > + except KeyError: > + # Fall through to "default" > + pass so me try except… use either membership testing: if default == 'push' and 'default-push' in self: return self['default-push'] or use `get`: if default == 'push': val = self.get('default-push') if val is not None: return val > + > + if default: > + try: > + return self['default'] > + except KeyError: > + pass same here.
Patch
diff --git a/mercurial/ui.py b/mercurial/ui.py --- a/mercurial/ui.py +++ b/mercurial/ui.py @@ -531,9 +531,16 @@ class ui(object): """Return repository location relative to cwd or from [paths]""" if util.hasscheme(loc) or os.path.isdir(os.path.join(loc, '.hg')): return loc - p = self.paths.getpath(loc, default=default) + if loc == 'default-push': + p = self.paths.getpath(loc, default=True) + elif loc == 'default': + p = self.paths.getpath(loc) + else: + assert default != 'default-push' + p = self.paths.getpath(loc, default=default=='default') + if p: return p.loc return loc @@ -949,22 +956,34 @@ class paths(dict): continue self[name] = path(name, rawloc=loc) - def getpath(self, name, default=None): + def getpath(self, name, default=False): """Return a ``path`` for the specified name, falling back to a default. - Returns the first of ``name`` or ``default`` that is present, or None - if neither is present. + If ``default`` is True, we attempt to resolve the default path. + If ``default`` is the string ``push``, we attempt to resolve the + default push path. + + Returns ``None`` if a path could not be found. """ try: return self[name] except KeyError: - if default is not None: - try: - return self[default] - except KeyError: - pass + pass + + if default == 'push': + try: + return self['default-push'] + except KeyError: + # Fall through to "default" + pass + + if default: + try: + return self['default'] + except KeyError: + pass return None class path(object):