Issue #3702: Allow single character names in local variables, method and catch-blocks parameters names in accordance with Google Style Guide #3774

MEZk · 2017-01-28T21:35:14Z

#3702

romani · 2017-01-28T21:53:20Z

diff report on guava is required.

please remove fix for Abbreviation, as 0 value lead to unclear violation message. See #3721 .

MEZk · 2017-01-30T17:53:30Z

@romani

diff report on guava is required.

Done. See http://mezk.github.io/i3702-google-config/diff
There are two types of differences:

Due to the new regexp format (the meaning of the violations is the same)
Due to allowance of one-character names.

Diff reports looks OK as for me.

please remove fix for Abbreviation, as 0 value lead to unclear violation message.

Done

I don't know why codecov is failing.

codecov-io · 2017-01-30T18:06:17Z

Codecov Report

Merging #3774 into master will not change coverage.

@@          Coverage Diff           @@
##           master   #3774   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         275     275           
  Lines       13618   13618           
  Branches     3066    3066           
======================================
  Hits        13618   13618

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dab7a9a...bb49c2f. Read the comment docs.

MEZk · 2017-01-31T05:51:55Z

@romani
CIs are green.

romani

@MEZk , please address my requests

romani · 2017-02-02T03:43:03Z

...com/google/checkstyle/test/chapter5naming/rule51identifiernames/InputCatchParameterName.java

@@ -21,7 +21,7 @@
        } catch (Exception noWorries) { // ok
        }
        try {
-        } catch (Throwable t) { // warn
+        } catch (Throwable t) {


please put "// ok" instead, not sure why we did this .... but lets keep this.

@romani
Done.

romani · 2017-02-02T03:45:41Z

src/main/resources/google_checks.xml

            <property name="accessModifiers" value="protected, package, private"/>
            <message key="name.invalidPattern"
             value="Parameter name ''{0}'' must match pattern ''{1}''."/>
        </module>
        <module name="ParameterName">
            <property name="id" value="ParameterNamePublic"/>
-            <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
+            <property name="format" value="^[a-z]([a-z0-9][A-Z]?)+$"/>


I am not sure why non-public and public regexps are different. Google confirmed two times in issue on google style that checkstyle rule should be relaxed. It is their style - so we follow it. If smb do not like this (I hope most of projects) they will change this in their configuration.

accessModifiers should be removed from google style config.

@romani
Google Style Guide prohibits one-character parameter names for public methods and allows them for non-public methods.
See https://google.github.io/styleguide/javaguide.html#s5.2.6-parameter-names (the second sentence).
That is why we have 2 different regexpses. I read the thread and I don't know whether it is OK that they do not follow their own guide.

Please read the issue discussion on Google style guide google/styleguide#214 (comment)

@romani Should we re-open issue and request the documentation be changed to prevent any more confusion?
We have said before we only go by that documentation like when they requested us to validate against another tool.

Reopen will not work, let's do reopen and PR to propose the fix

@romani @rnveach
I opened the issue: google/styleguide#226
PR: google/styleguide#227

rnveach · 2017-02-02T14:42:40Z

.../com/google/checkstyle/test/chapter5naming/rule51identifiernames/CatchParameterNameTest.java

            "47:28: " + getCheckMessage(checkConfig.getMessages(), msgKey, "iException", format),
-            "50:28: " + getCheckMessage(checkConfig.getMessages(), msgKey, "x", format),


One example of a violation isn't much. Can we change or add more examples?

@rnveach
Done.

romani · 2017-02-02T22:07:54Z

accessModifiers usage has to be removed.

MEZk · 2017-02-03T04:43:42Z

@romani

accessModifiers usage has to be removed.

Done. CIs are gree.
New diff report: http://mezk.github.io/i3702-google-config/diff
Issue: google/styleguide#226
PR: google/styleguide#227

MEZk · 2017-02-06T18:22:17Z

@romani @rnveach
Are you OK with the changes?

rnveach · 2017-02-06T18:29:38Z

If we merge this we will be out of sync with the google document on purpose for this one item as style team has not responded to Issue or PR.

romani

please regenerate reports to make sure that my last request do not have affect validation.

romani · 2017-02-06T23:01:09Z

src/main/resources/google_checks.xml

            <message key="name.invalidPattern"
             value="Catch parameter name ''{0}'' must match pattern ''{1}''."/>
        </module>
        <module name="LocalVariableName">
            <property name="tokens" value="VARIABLE_DEF"/>
-            <property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9]*$"/>
+            <property name="format" value="^[a-z]([a-z0-9]+[A-Z]?)*$"/>
            <property name="allowOneCharVarInForLoop" value="true"/>


looks like we do not need this property allowOneCharVarInForLoop anymore , please remove

@romani
Done

MEZk · 2017-02-07T19:44:08Z

@romani

please regenerate reports to make sure that my last request do not have affect validation.

DIff reports:

romani · 2017-02-09T16:47:31Z

@MEZk , reports are ok, but they are show that you change config in the way that is not requested in issue.
You changed a format to address abbreviations, that was not requested in issue.
If you think it also should be done - please create separate issue on this. Do not mix all fixes together!

config regexps should be fixed
reports need to be regenerated

MEZk · 2017-02-09T20:16:45Z

@romani

config regexps should be fixed

Done. Config was changed to allow one-characters names. ParameterNameCheck, LocalVariableCheck and CatchParameterCheck should not validate abbreviations in identifiers names. Abbreviations should be validated by special check AbbreviationAsWordInName. I created separate issue to fix this: #3812

reports need to be regenerated

Done. See http://mezk.github.io/i3702-google-config/diff/

…s, method and catch-blocks parameters names in accordance with Google Style Guide

romani · 2017-02-09T21:05:58Z

I canceled MacOS build manually (performance reason), it is ok to skip it for this PR.
code changes is ok, config changes is ok.
report is good.

@rnveach , please review one more time and do merge if all is fine to your mind.

MEZk force-pushed the i3702-google-style-naming branch from 9393a7f to b898f2e Compare January 30, 2017 17:51

MEZk force-pushed the i3702-google-style-naming branch 2 times, most recently from 6fef764 to c1d9ad5 Compare January 31, 2017 04:53

romani requested changes Feb 2, 2017

View reviewed changes

MEZk force-pushed the i3702-google-style-naming branch from c1d9ad5 to ba0f8da Compare February 2, 2017 04:52

rnveach reviewed Feb 2, 2017

View reviewed changes

MEZk mentioned this pull request Feb 2, 2017

Clearly articulate the rule for public methods parameter names in section 5.2.6 of Google Java Style Guide google/styleguide#226

Open

MEZk force-pushed the i3702-google-style-naming branch from ba0f8da to 770cfb9 Compare February 2, 2017 19:51

MEZk force-pushed the i3702-google-style-naming branch from 770cfb9 to bf88b0f Compare February 4, 2017 08:08

MEZk mentioned this pull request Feb 4, 2017

diff.groovy tool should have options to specify base config and patch config checkstyle/contribution#167

Closed

MEZk force-pushed the i3702-google-style-naming branch from bf88b0f to ab5529f Compare February 4, 2017 08:22

rnveach approved these changes Feb 6, 2017

View reviewed changes

romani requested changes Feb 6, 2017

View reviewed changes

MEZk force-pushed the i3702-google-style-naming branch from ab5529f to 32c1fc7 Compare February 7, 2017 19:41

MEZk force-pushed the i3702-google-style-naming branch from 32c1fc7 to cfb94be Compare February 9, 2017 20:01

Issue checkstyle#3702: Allow single character names in local variable…

bb49c2f

…s, method and catch-blocks parameters names in accordance with Google Style Guide

MEZk force-pushed the i3702-google-style-naming branch from cfb94be to bb49c2f Compare February 9, 2017 20:21

romani approved these changes Feb 9, 2017

View reviewed changes

romani mentioned this pull request Feb 9, 2017

patch-diff-report-tool: make ability to show lines as yellow if changes is only in message checkstyle/contribution#186

Open

rnveach merged commit 3e54123 into checkstyle:master Feb 9, 2017

MEZk deleted the i3702-google-style-naming branch February 12, 2017 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3702: Allow single character names in local variables, method and catch-blocks parameters names in accordance with Google Style Guide #3774

Issue #3702: Allow single character names in local variables, method and catch-blocks parameters names in accordance with Google Style Guide #3774

MEZk commented Jan 28, 2017

romani commented Jan 28, 2017

MEZk commented Jan 30, 2017 •

edited

codecov-io commented Jan 30, 2017 •

edited

MEZk commented Jan 31, 2017

romani left a comment

romani Feb 2, 2017

MEZk Feb 2, 2017

romani Feb 2, 2017

MEZk Feb 2, 2017 •

edited

romani Feb 2, 2017

rnveach Feb 2, 2017 •

edited

romani Feb 2, 2017

MEZk Feb 2, 2017 •

edited

rnveach Feb 2, 2017

MEZk Feb 2, 2017

romani commented Feb 2, 2017

MEZk commented Feb 3, 2017 •

edited

MEZk commented Feb 6, 2017

rnveach commented Feb 6, 2017 •

edited

romani left a comment

romani Feb 6, 2017

MEZk Feb 7, 2017

MEZk commented Feb 7, 2017

romani commented Feb 9, 2017

MEZk commented Feb 9, 2017 •

edited

romani commented Feb 9, 2017

		"47:28: " + getCheckMessage(checkConfig.getMessages(), msgKey, "iException", format),
		"50:28: " + getCheckMessage(checkConfig.getMessages(), msgKey, "x", format),

Issue #3702: Allow single character names in local variables, method and catch-blocks parameters names in accordance with Google Style Guide #3774

Issue #3702: Allow single character names in local variables, method and catch-blocks parameters names in accordance with Google Style Guide #3774

Conversation

MEZk commented Jan 28, 2017

romani commented Jan 28, 2017

MEZk commented Jan 30, 2017 • edited

codecov-io commented Jan 30, 2017 • edited

Codecov Report

MEZk commented Jan 31, 2017

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MEZk Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MEZk Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Feb 2, 2017

MEZk commented Feb 3, 2017 • edited

MEZk commented Feb 6, 2017

rnveach commented Feb 6, 2017 • edited

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MEZk commented Feb 7, 2017

romani commented Feb 9, 2017

MEZk commented Feb 9, 2017 • edited

romani commented Feb 9, 2017

MEZk commented Jan 30, 2017 •

edited

codecov-io commented Jan 30, 2017 •

edited

MEZk Feb 2, 2017 •

edited

rnveach Feb 2, 2017 •

edited

MEZk Feb 2, 2017 •

edited

MEZk commented Feb 3, 2017 •

edited

rnveach commented Feb 6, 2017 •

edited

MEZk commented Feb 9, 2017 •

edited