Patchwork D2010: check-commit: allow foo_bar naming in functions

login
register
mail settings
Submitter phabricator
Date Feb. 2, 2018, 6:14 p.m.
Message ID <differential-rev-PHID-DREV-igqtshpvxdznplee7uko-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27238/
State New
Headers show

Comments

phabricator - Feb. 2, 2018, 6:14 p.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  nameswithallthewordssmashedtogetherarehardtoread.
  especiallyifenglishisnotyourprimarylanguage.
  
  Let's align with the rest of the programming universe and
  allow_the_use_of_underscores_in_names.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  contrib/check-code.py
  contrib/check-commit
  tests/test-contrib-check-commit.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 2, 2018, 6:15 p.m.
pulkit accepted this revision.
pulkit added a comment.


  I am +1 on this.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - Feb. 2, 2018, 6:17 p.m.
durin42 accepted this revision as: durin42.
durin42 added a comment.


  I'm also +1 on this.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: durin42, pulkit, mercurial-devel
phabricator - Feb. 3, 2018, 8:35 a.m.
yuja added a comment.


  I'm kinda -1 on this because of inconsistency with the 10-year codebase.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: yuja, durin42, pulkit, mercurial-devel
phabricator - Feb. 4, 2018, 5:46 a.m.
av6 added a comment.


  As opposed to 11 words smashed together, function names in Mercurial usually are 2 or 3 words (if not just one), which is not at all bad. I think breaking inconsistency with the existing code base would be worse than making people get used to no-spaces style (the one that python's stdlib usually follows, in fact). So, FWIW, -1 from me.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: av6, yuja, durin42, pulkit, mercurial-devel
phabricator - March 15, 2019, 1:37 a.m.
indygreg added a comment.


  Should we queue this patch or abandon it?

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: av6, yuja, durin42, pulkit, mercurial-devel
phabricator - March 15, 2019, 1:41 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2010#89337, @indygreg wrote:
  
  > Should we queue this patch or abandon it?
  
  
  I wanted to have underscores in names but reading @yuja and @av6 comments about inconsistency, I feel maybe it's not a good idea.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: av6, yuja, durin42, pulkit, mercurial-devel
phabricator - March 15, 2019, 3:24 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2010#89337, @indygreg wrote:
  
  > Should we queue this patch or abandon it?
  
  
  I'm for it, even though it leads to inconsistency. However, we may want to discuss ahead of time what our long-term plan for existing symbols is. Do we eventually want to remove that inconsistency? I took a quick look for examples where it seemed obviously not worth it to rename and it was harder to find good examples than I had expected. Perhaps `bail_if_changed` and `extensions.wrap_function` are some of the more frequently used. But most very commonly used functions seem to have short names already. So maybe even if we wanted to eventually make it consistent, it won't be as bad as people have feared? I still don't feel very strongly, but I wanted to highlight what it would mean in practice.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: martinvonz, av6, yuja, durin42, pulkit, mercurial-devel
phabricator - March 15, 2019, 4:31 p.m.
indygreg added a comment.


  If we loosen the naming requirement, I think a good convention would be to have new files use the *modern* convention and for existing code/files to generally stick to the old convention.
  
  That being said, if someone were to introduce a new function into an existing file and wanted to use the modern names, I wouldn't mind.
  
  I would not like to see global, API breaking rewrites for the sake of rewrites. If we wanted to do a global search and replace on variables inside functions, I'd be OK with that (that won't break API compat). But I'm in no rush to do it.
  
  I would also not like to see patches introducing mixed naming conventions within functions. I think we should try to keep things consistent at definitely the function level and possibly the file level.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: martinvonz, av6, yuja, durin42, pulkit, mercurial-devel
phabricator - March 15, 2019, 4:37 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D2010#89436, @indygreg wrote:
  
  > If we loosen the naming requirement, I think a good convention would be to have new files use the *modern* convention and for existing code/files to generally stick to the old convention.
  >
  > That being said, if someone were to introduce a new function into an existing file and wanted to use the modern names, I wouldn't mind.
  >
  > I would not like to see global, API breaking rewrites for the sake of rewrites. If we wanted to do a global search and replace on variables inside functions, I'd be OK with that (that won't break API compat). But I'm in no rush to do it.
  >
  > I would also not like to see patches introducing mixed naming conventions within functions. I think we should try to keep things consistent at definitely the function level and possibly the file level.
  
  
  Sounds good to me. As I said before, we don't seem to have that many functions that have many words in their name, so I don't think the inconsistency would be very noticeable anyway.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: martinvonz, av6, yuja, durin42, pulkit, mercurial-devel
phabricator - March 16, 2019, 10:28 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2010#89433, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D2010#89337, @indygreg wrote:
  >
  > > Should we queue this patch or abandon it?
  >
  >
  > I'm for it, even though it leads to inconsistency. However, we may want to discuss ahead of time what our long-term plan for existing symbols is. Do we eventually want to remove that inconsistency? I took a quick look for examples where it seemed obviously not worth it to rename and it was harder to find good examples than I had expected. Perhaps `bail_if_changed` and `extensions.wrap_function` are some of the more frequently used. But most very commonly used functions seem to have short names already. So maybe even if we wanted to eventually make it consistent, it won't be as bad as people have feared? I still don't feel very strongly, but I wanted to highlight what it would mean in practice.
  
  
  extensions.wrap_function is one of the most used function by extensions and I will be happy if we don't rename it. Renaming it will break a lot of extensions, let's prevent it.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers, pulkit, durin42
Cc: martinvonz, av6, yuja, durin42, pulkit, mercurial-devel

Patch

diff --git a/tests/test-contrib-check-commit.t b/tests/test-contrib-check-commit.t
--- a/tests/test-contrib-check-commit.t
+++ b/tests/test-contrib-check-commit.t
@@ -132,8 +132,6 @@ 
    This has no topic and ends with a period.
   19: adds double empty line
    +
-  20: adds a function with foo_bar naming
-   + def blah_blah(x):
   23: adds double empty line
    +
   [1]
diff --git a/contrib/check-commit b/contrib/check-commit
--- a/contrib/check-commit
+++ b/contrib/check-commit
@@ -41,12 +41,6 @@ 
     (afterheader + r".{79,}", "summary line too long (limit is 78)"),
     (r"\n\+\n( |\+)\n", "adds double empty line"),
     (r"\n \n\+\n", "adds double empty line"),
-    # Forbid "_" in function name.
-    #
-    # We skip the check for cffi related functions. They use names mapping the
-    # name of the C function. C function names may contain "_".
-    (r"\n\+[ \t]+def (?!cffi)[a-z]+_[a-z]",
-     "adds a function with foo_bar naming"),
 ]
 
 word = re.compile('\S')
diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -263,8 +263,6 @@ 
     (r'.{81}', "line too long"),
     (r'[^\n]\Z', "no trailing newline"),
     (r'(\S[ \t]+|^[ \t]+)\n', "trailing whitespace"),
-#    (r'^\s+[^_ \n][^_. \n]+_[^_\n]+\s*=',
-#     "don't use underbars in identifiers"),
     (r'^\s+(self\.)?[A-Za-z][a-z0-9]+[A-Z]\w* = ',
      "don't use camelcase in identifiers", r'#.*camelcase-required'),
     (r'^\s*(if|while|def|class|except|try)\s[^[\n]*:\s*[^\\n]#\s]+',