Patchwork D7084: fix: make Fixer initialization more explicit for clarity

login
register
mail settings
Submitter phabricator
Date Oct. 14, 2019, 6:39 a.m.
Message ID <differential-rev-PHID-DREV-wmbenu5bkagbbveetnq6-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/42313/
State Superseded
Headers show

Comments

phabricator - Oct. 14, 2019, 6:39 a.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I found it quite confusing that Fixer accessed fields that seemed like
  they didn't exist.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7084

AFFECTED FILES
  hgext/fix.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 15, 2019, 12:11 p.m.
pulkit added a comment.


  Hm, IIUC this was edited in flight, not sure why.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7084/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7084

To: martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel
phabricator - Oct. 15, 2019, 12:53 p.m.
martinvonz added a comment.


  In D7084#104166 <https://phab.mercurial-scm.org/D7084#104166>, @pulkit wrote:
  
  > Hm, IIUC this was edited in flight, not sure why.
  
  I noticed that test-check-format.t was failing last night so I fixed it up in flight. It wanted to put the procutil import on one line.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7084/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7084

To: martinvonz, #hg-reviewers, pulkit
Cc: mercurial-devel

Patch

diff --git a/hgext/fix.py b/hgext/fix.py
--- a/hgext/fix.py
+++ b/hgext/fix.py
@@ -132,11 +132,9 @@ 
 from mercurial.i18n import _
 from mercurial.node import nullrev
 from mercurial.node import wdirrev
-from mercurial.pycompat import setattr
 
 from mercurial.utils import (
     procutil,
-    stringutil,
 )
 
 from mercurial import (
@@ -172,9 +170,9 @@ 
     b'linerange': None,
     b'pattern': None,
     b'priority': 0,
-    b'metadata': b'false',
-    b'skipclean': b'true',
-    b'enabled': b'true',
+    b'metadata': False,
+    b'skipclean': True,
+    b'enabled': True,
 }
 
 for key, default in FIXER_ATTRS.items():
@@ -793,29 +791,27 @@ 
     """
     fixers = {}
     for name in fixernames(ui):
-        fixers[name] = Fixer()
-        for key in FIXER_ATTRS:
-            setattr(
-                fixers[name],
-                pycompat.sysstr(b'_' + key),
-                ui.config(b'fix', name + b':' + key),
-            )
-        fixers[name]._priority = int(fixers[name]._priority)
-        fixers[name]._metadata = stringutil.parsebool(fixers[name]._metadata)
-        fixers[name]._skipclean = stringutil.parsebool(fixers[name]._skipclean)
-        fixers[name]._enabled = stringutil.parsebool(fixers[name]._enabled)
+        enabled = ui.configbool(b'fix', name + b':enabled')
+        command = ui.config(b'fix', name + b':command')
+        pattern = ui.config(b'fix', name + b':pattern')
+        linerange = ui.config(b'fix', name + b':linerange')
+        priority = ui.configint(b'fix', name + b':priority')
+        metadata = ui.configbool(b'fix', name + b':metadata')
+        skipclean = ui.configbool(b'fix', name + b':skipclean')
         # Don't use a fixer if it has no pattern configured. It would be
         # dangerous to let it affect all files. It would be pointless to let it
         # affect no files. There is no reasonable subset of files to use as the
         # default.
-        if fixers[name]._pattern is None:
+        if pattern is None:
             ui.warn(
                 _(b'fixer tool has no pattern configuration: %s\n') % (name,)
             )
-            del fixers[name]
-        elif not fixers[name]._enabled:
+        elif not enabled:
             ui.debug(b'ignoring disabled fixer tool: %s\n' % (name,))
-            del fixers[name]
+        else:
+            fixers[name] = Fixer(
+                command, pattern, linerange, priority, metadata, skipclean
+            )
     return collections.OrderedDict(
         sorted(fixers.items(), key=lambda item: item[1]._priority, reverse=True)
     )
@@ -833,6 +829,16 @@ 
 class Fixer(object):
     """Wraps the raw config values for a fixer with methods"""
 
+    def __init__(
+        self, command, pattern, linerange, priority, metadata, skipclean
+    ):
+        self._command = command
+        self._pattern = pattern
+        self._linerange = linerange
+        self._priority = priority
+        self._metadata = metadata
+        self._skipclean = skipclean
+
     def affects(self, opts, fixctx, path):
         """Should this fixer run on the file at the given path and context?"""
         return self._pattern is not None and scmutil.match(