[PATCH 2 of 3] upgrade: move descriptions and selection logic in individual classes

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sun Apr 16 19:27:59 EDT 2017



On 04/16/2017 03:18 PM, Yuya Nishihara wrote:
> On Thu, 13 Apr 2017 01:12:15 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
>> # Date 1492007645 -7200
>> #      Wed Apr 12 16:34:05 2017 +0200
>> # Node ID 346b07413137d7f56196941212958df03e55b202
>> # Parent  b42c1f35aedd153c77741d54dac50aa6d3ea43e2
>> # EXP-Topic upgraderepo
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 346b07413137
>> upgrade: move descriptions and selection logic in individual classes
>>
>> Our goal here is to get top level definition for all the format variants. Having
>> them defined outside of the function enabled other users of that logic.
>>
>> They are two keys components of a format variant:
>>
>> 1) the name and various descriptions of its effect,
>>
>> 2) the code that checks if the repo is using this variant and if the config
>>    enables it.
>>
>> That second items make us pick a class-based approach, since different variants
>> requires different code (even if in practice, many can reuse the same logic).
>> Each variants define its own class that is then used like a singleton.  The
>> class-based approach also clarify the definitions part a bit since each are
>> simple assignment in an indented block.
>
> Since I'm not a fan of using class as a syntactic sugar, I prefer a plain
> function that creates an improvement object. But I have no strong option
> about that.

The main motivation to use class is the distinct code (and sometimes 
attribute) for each variants, having a "builder" function would make the 
code handling of the code much more hacky.

I am not fully happy about the current state, but I do not see a clean 
alternative yet. Either I'm missing something in your suggestion or I 
find it hackier.


>> -class formatvariant(improvement):
>> -    """an improvement subclass dedicated to repository format
>> +class formatvariant(object):
>> +    """an improvement subclass dedicated to repository format"""
>
> Nit: it's no longer be a subclass of improvement.

Gah, bad merge resolution.

I can send a V2 or a follow up as you prefer. (given how near the freeze 
is, a follow up would seems better)

>
>> +    type = deficiency
>> +    ### The following attributes should be defined for each class:
>> +
>> +    # machine-readable string uniquely identifying this improvement. it will be
>> +    # mapped to an action later in the upgrade process.
>> +    name = None
>
> Confirmed that the formatvariant class doesn't need __eq__ because it won't be
> instantiated.

Yep. It does not needs one.

>> +class fncache(requirementformatvariant):
>> +    name = 'fncache'
>> +
>> +    _requirement = 'fncache'
>> +
>> +    default = True
>> +
>> +    description = _('long and reserved filenames may not work correctly; '
>> +                    'repository performance is sub-optimal')
>> +
>> +    upgrademessage=_('repository will be more resilient to storing '
>> +                     'certain paths and performance of certain '
>> +                     'operations should be improved')
>
> Missing spaces around '='. The same formatting issues are seen in several
> places.

Good cache, I can send a V2 or a follow up as you prefer.

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list