Submitter | Josef 'Jeff' Sipek |
---|---|
Date | March 27, 2020, 2:48 p.m. |
Message ID | <f313b33e0341724093d8.1585320538@meili> |
Download | mbox | patch |
Permalink | /patch/45922/ |
State | Accepted |
Headers | show |
Comments
On Fri, Mar 27, 2020 at 10:48:58 -0400, Josef 'Jeff' Sipek wrote: > # HG changeset patch > # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net> > # Date 1585319999 14400 > # Fri Mar 27 10:39:59 2020 -0400 > # Node ID f313b33e0341724093d866f0bf5478a28cad092a > # Parent 4f4c2622ec748ce806c6577c30d4f1ae8660a0c2 > pathutil: document that dirs map type implies manifest/dirstate processing FWIW, this "argument type implies manifest or dirstate" seems like a hack. I'm not familiar enough with python to know if I'm wrong here, but I'm open to replacing this patch with somethig that adds a "type" argument. Then, __init__ can assert/abort/etc. if it gets a mismatch. I imagine something like: def __init__(self, map, is_dirstate, skip=None): if is_dirstate: assert isinstance(map, dict) else: assert isinstance(map, list) ...code more or less as before... Would this be a good change? Thanks, Jeff. > > diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py > --- a/mercurial/pathutil.py > +++ b/mercurial/pathutil.py > @@ -286,6 +286,9 @@ class dirs(object): > '''a multiset of directory names from a set of file paths''' > > def __init__(self, map, skip=None): > + ''' > + a dict map indicates a dirstate while a list indicates a manifest > + ''' > self._dirs = {} > addpath = self.addpath > if isinstance(map, dict) and skip is not None: > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
queued, thanks > On Mar 27, 2020, at 10:48, Josef 'Jeff' Sipek <jeffpc@josefsipek.net> wrote: > > # HG changeset patch > # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net> > # Date 1585319999 14400 > # Fri Mar 27 10:39:59 2020 -0400 > # Node ID f313b33e0341724093d866f0bf5478a28cad092a > # Parent 4f4c2622ec748ce806c6577c30d4f1ae8660a0c2 > pathutil: document that dirs map type implies manifest/dirstate processing > > diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py > --- a/mercurial/pathutil.py > +++ b/mercurial/pathutil.py > @@ -286,6 +286,9 @@ class dirs(object): > '''a multiset of directory names from a set of file paths''' > > def __init__(self, map, skip=None): > + ''' > + a dict map indicates a dirstate while a list indicates a manifest > + ''' > self._dirs = {} > addpath = self.addpath > if isinstance(map, dict) and skip is not None: > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> On Mar 27, 2020, at 10:56, Josef 'Jeff' Sipek <jeffpc@josefsipek.net> wrote: > > On Fri, Mar 27, 2020 at 10:48:58 -0400, Josef 'Jeff' Sipek wrote: >> # HG changeset patch >> # User Josef 'Jeff' Sipek <jeffpc@josefsipek.net> >> # Date 1585319999 14400 >> # Fri Mar 27 10:39:59 2020 -0400 >> # Node ID f313b33e0341724093d866f0bf5478a28cad092a >> # Parent 4f4c2622ec748ce806c6577c30d4f1ae8660a0c2 >> pathutil: document that dirs map type implies manifest/dirstate processing > > FWIW, this "argument type implies manifest or dirstate" seems like a hack. > I'm not familiar enough with python to know if I'm wrong here, but I'm open > to replacing this patch with somethig that adds a "type" argument. Then, > __init__ can assert/abort/etc. if it gets a mismatch. I imagine something > like: > > def __init__(self, map, is_dirstate, skip=None): > if is_dirstate: > assert isinstance(map, dict) > else: > assert isinstance(map, list) > ...code more or less as before... > > Would this be a good change? I'm conflicted. What we've got is akin to a type-overload in C++, but there's (obviously) no function overloading in Python. It might be clearer to convert this to three methods, eg (PEP484 hints included for clarity): def __init__(self, seq: Iterable[bytes]): for f in seq: addpath(f) @classmethod def fromdirstate(cls, dirstate_map: Dict[bytes, T], skip=None): if skip is not None: return cls(f for f, s in pycompat.iteritems(dirstate_map) if s[0] != skip) return cls(dirstate) @classmethod def frommanifest(cls, files: Iterable[bytes]): return cls(files) You could probably even elide the second one, since it's more obvious what's going on. Thoughts? > > Thanks, > > Jeff. > >> >> diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py >> --- a/mercurial/pathutil.py >> +++ b/mercurial/pathutil.py >> @@ -286,6 +286,9 @@ class dirs(object): >> '''a multiset of directory names from a set of file paths''' >> >> def __init__(self, map, skip=None): >> + ''' >> + a dict map indicates a dirstate while a list indicates a manifest >> + ''' >> self._dirs = {} >> addpath = self.addpath >> if isinstance(map, dict) and skip is not None: >> _______________________________________________ >> Mercurial-devel mailing list >> Mercurial-devel@mercurial-scm.org >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel > > -- > You measure democracy by the freedom it gives its dissidents, not the > freedom it gives its assimilated conformists. > - Abbie Hoffman > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Patch
diff --git a/mercurial/pathutil.py b/mercurial/pathutil.py --- a/mercurial/pathutil.py +++ b/mercurial/pathutil.py @@ -286,6 +286,9 @@ class dirs(object): '''a multiset of directory names from a set of file paths''' def __init__(self, map, skip=None): + ''' + a dict map indicates a dirstate while a list indicates a manifest + ''' self._dirs = {} addpath = self.addpath if isinstance(map, dict) and skip is not None: