Patchwork [1,of,3] upgrade: introduce a 'formatvariant' class

login
register
mail settings
Submitter Pierre-Yves David
Date April 12, 2017, 11:12 p.m.
Message ID <b42c1f35aedd153c7774.1492038734@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20143/
State Superseded
Headers show

Comments

Pierre-Yves David - April 12, 2017, 11:12 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1491860083 -7200
#      Mon Apr 10 23:34:43 2017 +0200
# Node ID b42c1f35aedd153c77741d54dac50aa6d3ea43e2
# Parent  3c77f03f16b386940c60af36d6a3ad83bee37ad4
# 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 b42c1f35aedd
upgrade: introduce a 'formatvariant' class

The 'deficiency' type has multiple specificities. We create a dedicated class to
host them. More logic will be added incrementally in future changesets.
Yuya Nishihara - April 16, 2017, 1:06 p.m.
On Thu, 13 Apr 2017 01:12:14 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1491860083 -7200
> #      Mon Apr 10 23:34:43 2017 +0200
> # Node ID b42c1f35aedd153c77741d54dac50aa6d3ea43e2
> # Parent  3c77f03f16b386940c60af36d6a3ad83bee37ad4
> # 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 b42c1f35aedd
> upgrade: introduce a 'formatvariant' class

The code looks generally good, but I'll wait for some comments from Greg.
A couple of nits inline.

> --- a/mercurial/upgrade.py
> +++ b/mercurial/upgrade.py
> @@ -120,6 +120,23 @@ class improvement(object):
>      upgrademessage
>         Message intended for humans explaining what an upgrade addressing this
>         issue will do. Should be worded in the future tense.
> +    """
> +    def __init__(self, name, type, description, upgrademessage):
> +        self.name = name
> +        self.type = type
> +        self.description = description
> +        self.upgrademessage = upgrademessage
> +
> +    def __eq__(self, other):
> +        if not isinstance(other, improvement):
> +            # This is what python tell use to do
> +            return NotImplemented
> +        return self.name == other.name

This isn't a bug introduced by this patch, but __ne__ and __hash__ should
also be defined.

> +    def __init__(self, name, description, upgrademessage, fromdefault,
> +                 fromconfig):
> +        super(formatvariant, self).__init__(name, 'deficiency', description,
> +                                            upgrademessage)

Better to use the "deficiency" constant, but I don't care much since __init__
will be removed by the next patch.
Pierre-Yves David - April 16, 2017, 11:30 p.m.
On 04/16/2017 03:06 PM, Yuya Nishihara wrote:
> On Thu, 13 Apr 2017 01:12:14 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1491860083 -7200
>> #      Mon Apr 10 23:34:43 2017 +0200
>> # Node ID b42c1f35aedd153c77741d54dac50aa6d3ea43e2
>> # Parent  3c77f03f16b386940c60af36d6a3ad83bee37ad4
>> # 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 b42c1f35aedd
>> upgrade: introduce a 'formatvariant' class
>
> The code looks generally good, but I'll wait for some comments from Greg.
> A couple of nits inline.
>
>> --- a/mercurial/upgrade.py
>> +++ b/mercurial/upgrade.py
>> @@ -120,6 +120,23 @@ class improvement(object):
>>      upgrademessage
>>         Message intended for humans explaining what an upgrade addressing this
>>         issue will do. Should be worded in the future tense.
>> +    """
>> +    def __init__(self, name, type, description, upgrademessage):
>> +        self.name = name
>> +        self.type = type
>> +        self.description = description
>> +        self.upgrademessage = upgrademessage
>> +
>> +    def __eq__(self, other):
>> +        if not isinstance(other, improvement):
>> +            # This is what python tell use to do
>> +            return NotImplemented
>> +        return self.name == other.name
>
> This isn't a bug introduced by this patch, but __ne__ and __hash__ should
> also be defined.

Ah, good catch, do you want a V2 or a follow up ?

>
>> +    def __init__(self, name, description, upgrademessage, fromdefault,
>> +                 fromconfig):
>> +        super(formatvariant, self).__init__(name, 'deficiency', description,
>> +                                            upgrademessage)
>
> Better to use the "deficiency" constant, but I don't care much since __init__
> will be removed by the next patch.

Good catch. (will be in V2/followup)

Patch

diff --git a/mercurial/upgrade.py b/mercurial/upgrade.py
--- a/mercurial/upgrade.py
+++ b/mercurial/upgrade.py
@@ -120,6 +120,23 @@  class improvement(object):
     upgrademessage
        Message intended for humans explaining what an upgrade addressing this
        issue will do. Should be worded in the future tense.
+    """
+    def __init__(self, name, type, description, upgrademessage):
+        self.name = name
+        self.type = type
+        self.description = description
+        self.upgrademessage = upgrademessage
+
+    def __eq__(self, other):
+        if not isinstance(other, improvement):
+            # This is what python tell use to do
+            return NotImplemented
+        return self.name == other.name
+
+class formatvariant(improvement):
+    """an improvement subclass dedicated to repository format
+
+    extra attributes:
 
     fromdefault (``deficiency`` types only)
        Boolean indicating whether the current (deficient) state deviates
@@ -129,20 +146,13 @@  class improvement(object):
        Boolean indicating whether the current (deficient) state deviates
        from the current Mercurial configuration.
     """
-    def __init__(self, name, type, description, upgrademessage, **kwargs):
-        self.name = name
-        self.type = type
-        self.description = description
-        self.upgrademessage = upgrademessage
 
-        for k, v in kwargs.items():
-            setattr(self, k, v)
-
-    def __eq__(self, other):
-        if not isinstance(other, improvement):
-            # This is what python tell use to do
-            return NotImplemented
-        return self.name == other.name
+    def __init__(self, name, description, upgrademessage, fromdefault,
+                 fromconfig):
+        super(formatvariant, self).__init__(name, 'deficiency', description,
+                                            upgrademessage)
+        self.fromdefault = fromdefault
+        self.fromconfig = fromconfig
 
 def finddeficiencies(repo):
     """returns a list of deficiencies that the repo suffer from"""
@@ -155,9 +165,8 @@  def finddeficiencies(repo):
     # requirements, so let's not bother.
 
     if 'fncache' not in repo.requirements:
-        deficiencies.append(improvement(
+        deficiencies.append(formatvariant(
             name='fncache',
-            type=deficiency,
             description=_('long and reserved filenames may not work correctly; '
                           'repository performance is sub-optimal'),
             upgrademessage=_('repository will be more resilient to storing '
@@ -167,9 +176,8 @@  def finddeficiencies(repo):
             fromconfig='fncache' in newreporeqs))
 
     if 'dotencode' not in repo.requirements:
-        deficiencies.append(improvement(
+        deficiencies.append(formatvariant(
             name='dotencode',
-            type=deficiency,
             description=_('storage of filenames beginning with a period or '
                           'space may not work correctly'),
             upgrademessage=_('repository will be better able to store files '
@@ -178,9 +186,8 @@  def finddeficiencies(repo):
             fromconfig='dotencode' in newreporeqs))
 
     if 'generaldelta' not in repo.requirements:
-        deficiencies.append(improvement(
+        deficiencies.append(formatvariant(
             name='generaldelta',
-            type=deficiency,
             description=_('deltas within internal storage are unable to '
                           'choose optimal revisions; repository is larger and '
                           'slower than it could be; interaction with other '
@@ -202,9 +209,8 @@  def finddeficiencies(repo):
     for rev in cl:
         chainbase = cl.chainbase(rev)
         if chainbase != rev:
-            deficiencies.append(improvement(
+            deficiencies.append(formatvariant(
                 name='removecldeltachain',
-                type=deficiency,
                 description=_('changelog storage is using deltas instead of '
                               'raw entries; changelog reading and any '
                               'operation relying on changelog data are slower '