Patchwork D7464: filemerge: drop a default argument to appease pytype

login
register
mail settings
Submitter phabricator
Date Nov. 21, 2019, 3:27 a.m.
Message ID <differential-rev-PHID-DREV-3zjtqcmjaxtjgyr4rmbg-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43378/
State Superseded
Headers show

Comments

phabricator - Nov. 21, 2019, 3:27 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The function slices and takes the length of this argument without internally
  setting it if not provided.  There was no bug here because both callers passed
  the argument.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/filemerge.py

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Nov. 21, 2019, 8:07 a.m.
dlax added a comment.


  Shouldn't this be also done for all similar functions? (i.e. `_xmergeimm` and functions registered as a merge tool with `@internaltool`)

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Dec. 1, 2019, 6:38 p.m.
mharbison72 added a comment.


  In D7464#109795 <https://phab.mercurial-scm.org/D7464#109795>, @dlax wrote:
  
  > Shouldn't this be also done for all similar functions? (i.e. `_xmergeimm` and functions registered as a merge tool with `@internaltool`)
  
  I looked at this  a bit more, and it looks like the other functions either ignore the `labels` arg, or they handle the `None`/empty case to substitute something else.  So maybe that's a better thing to do.  But I'm not real familiar with merge code, and don't know what the default labels should be here.

REPOSITORY
  rHG Mercurial

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

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

To: mharbison72, #hg-reviewers
Cc: dlax, mercurial-devel

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -693,7 +693,7 @@ 
     ui.status(t.renderdefault(props))
 
 
-def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels=None):
+def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels):
     tool, toolpath, binary, symlink, scriptfn = toolconf
     uipathfn = scmutil.getuipathfn(repo)
     if fcd.isabsent() or fco.isabsent():