[PATCH 1 of 5] scmutil: rename classes from "opener" to "vfs"

[Adrian Buehlmann]

On 2012-08-13 08:17, FUJIWARA Katsunori wrote:
> 
> At Sun, 12 Aug 2012 11:09:59 +0200,
> Adrian Buehlmann wrote:
>>
>> On 2012-08-11 14:14, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>>> # Date 1344687074 -32400
>>> # Node ID dea54508bb39698cfd74b1281b0d12af09aff139
>>> # Parent  b131e24e2984a610d77e124dd3f58b2b5eda6d1a
>>> scmutil: rename classes from "opener" to "vfs"
>>
>> ..
>>
>>> This patch just rename class names, so still leaves original class
>>> names as aliases for renamed classes for backward compatibility.
>>
>> This sentence is erroneous. Perhaps this would work instead:
>>
>> "For backwards compatibility, aliases for the old names are added."
> 
> OK, I'll re-post with such description, thanks.
> 
>> Stupid question: Will the aliases be removed at some point in the
>> future? If yes, when will that happen?
> 
> IMHO, they should be removed:
> 
>   - just after all 'scmutil.*opener()' invocations are removed from
>     Mercurial core implementation("scmutil.opener()" is still invoked,
>     yet), or

[..]

>   - after the next major release of such removing, as grace period for
>     3rd party products

I don't think we've done such things in the past. There has never been
such a thing as a stable Mercurial API.

http://mercurial.selenic.com/wiki/MercurialApi#Why_you_shouldn.27t_use_Mercurial.27s_internal_API

The policy has always been that third party products have to cope with
moving Mercurial API (if they depend on Mercurial on that level).

So I think the only valid reason for "backwards compatibility" is thus a
temporal one: until you have migrated all uses.

>>> diff -r b131e24e2984 -r dea54508bb39 mercurial/scmutil.py
>>> --- a/mercurial/scmutil.py	Mon Aug 06 12:07:21 2012 -0500
>>> +++ b/mercurial/scmutil.py	Sat Aug 11 21:11:14 2012 +0900
>>> @@ -167,7 +167,7 @@
>>>          # want to add "foo/bar/baz" before checking if there's a "foo/.hg"
>>>          self.auditeddir.update(prefixes)
>>>  
>>> -class abstractopener(object):
>>> +class abstractvfs(object):
>>>      """Abstract base class; cannot be instantiated"""
>>>  
>>>      def __init__(self, *args, **kwargs):
>>> @@ -219,8 +219,10 @@
>>>      def makedirs(self, path=None, mode=None):
>>>          return util.makedirs(self.join(path), mode)
>>>  
>>> -class opener(abstractopener):
>>> -    '''Open files relative to a base directory
>>> +abstractopner = abstractvfs
>>              ^
>> Typo.
> 
> Ooops, I'll re-post fixed series.
> 
> "abstractopener" seems to be never referred from core implementation,
> after this patch :-)

So, why adding an alias then?

>>> --- a/mercurial/statichttprepo.py	Mon Aug 06 12:07:21 2012 -0500
>>> +++ b/mercurial/statichttprepo.py	Sat Aug 11 21:11:14 2012 +0900
>>> @@ -64,7 +64,7 @@
>>>      urlopener = url.opener(ui, authinfo)
>>>      urlopener.add_handler(byterange.HTTPRangeHandler())
>>>  
>>> -    class statichttpopener(scmutil.abstractopener):
>>> +    class statichttpvfs(scmutil.abstractvfs):
>>>          def __init__(self, base):
>>>              self.base = base
>>>  
>>> @@ -74,7 +74,7 @@
>>>              f = "/".join((self.base, urllib.quote(path)))
>>>              return httprangereader(f, urlopener)
>>>  
>>> -    return statichttpopener
>>> +    return statichttpvfs
>>>  
>>>  class statichttppeer(localrepo.localpeer):
>>>      def local(self):
>>
>> No alias for statichttpopener?
> 
> I think that "statichttpopener" name is never referred other that this
> function because it is defined function locally.
> 
>>> diff -r b131e24e2984 -r dea54508bb39 mercurial/store.py
>>> --- a/mercurial/store.py	Mon Aug 06 12:07:21 2012 -0500
>>> +++ b/mercurial/store.py	Sat Aug 11 21:11:14 2012 +0900
>>> @@ -367,7 +367,7 @@
>>>              self._load()
>>>          return iter(self.entries)
>>>  
>>> -class _fncacheopener(scmutil.abstractopener):
>>> +class _fncachevfs(scmutil.abstractvfs):
>>>      def __init__(self, op, fnc, encode):
>>>          self.opener = op
>>>          self.fncache = fnc
>>
>> ..
>>
>>> @@ -378,6 +378,8 @@
>>>              self.fncache.add(path)
>>>          return self.opener(self.encode(path), mode, *args, **kw)
>>>  
>>> +_fncacheopener = _fncachevfs
>>> +
>>
>> Is this really needed? _fncacheopener should be private to scmutil (it
>> starts with an underbar)
> 
> Yes, you are right. "_fncacheopener" shouldn't be referred from
> outside, of course.
> 
> But private-ness of "_fncacheopener" is ensured not by "restriction of
> Python" but by "agreement of programmers", and it is fact that
> "_fncacheopener" can be referred from outside of "store.py".
> 
> So, I treat it as same as other "opener" classes in this patch.

[..]

> I'll remove adding "_fncacheopener" alias at re-posting of this
> series, if this decision is bad.

I think you should only add an _fncacheopener alias if it is needed in
Mercurial's own code.

If it isn't needed in Mercurial's own code (includes core extensions,
obviously), then do not add the _fncacheopener alias.