Patchwork [2,of,2] pathutil: document that dirs map type implies manifest/dirstate processing

# 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

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