New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbbreviationAsWordInName: confusing violation message #3721

Closed
rnveach opened this Issue Jan 12, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jan 12, 2017

$ cat TestClass.java
public class TestClass {
    void shouldBeAloneOrSLine() {
    }
    void shouldBeAloneOrLine() {
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
    <module name="AbbreviationAsWordInName">
      <property name="ignoreFinal" value="false"/>
      <property name="allowedAbbreviationLength" value="0"/>
      <property name="allowedAbbreviations" value="AST"/>
    </module>
    </module>
</module>

$ java -jar checkstyle-7.4-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:2: Abbreviation in name 'shouldBeAloneOrSLine' must contain no more than '0' capital letters. [AbbreviationAsWordInName]
Audit done.
Checkstyle ends with 1 errors.

Violation message no more than '0' capital letters is confusing as one capital letter is allowed as seen in other method. It is more than 1 consecutive capital letter where violation occurs on.
I think just the message needs to be fixed, but then 0 for the config might be slightly confusing.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 12, 2017

Member

0 - abbreviations are not allowed.

But we need to find good wording for message to make human friendly.
Documentation should be extended too.

Member

romani commented Jan 12, 2017

0 - abbreviations are not allowed.

But we need to find good wording for message to make human friendly.
Documentation should be extended too.

@romani romani added the approved label Jan 12, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 12, 2017

Member

if we make no abbreviations in our code we need to resolve following problems, some of them are complicated , may that is why we stick with length of 0:

in main code:

api/FileContents.java:133:  'reportCComment'
api/FileContents.java:164:  'getCComments'
api/FileContents.java:176:  'extractCComment'
api/FileContents.java:319:  'hasIntersectionWithCComment'
checks/AvoidEscapedUnicodeCharactersCheck.java:295:  'isTrailingCComent'
checks/imports/CustomImportOrderCheck.java:787:  'getFirstNDomainsFromIdent'
checks/indentation/AbstractExpressionHandler.java:544:  'checkRParen'
checks/indentation/AbstractExpressionHandler.java:564:  'checkLParen'
checks/indentation/BlockParentHandler.java:136:  'getLCurly'
checks/indentation/BlockParentHandler.java:145:  'getRCurly'
checks/indentation/BlockParentHandler.java:153:  'checkLCurly'
checks/indentation/BlockParentHandler.java:185:  'checkRCurly'
checks/indentation/BlockParentHandler.java:229:  'getRParen'
checks/indentation/BlockParentHandler.java:238:  'getLParen'
checks/metrics/NPathComplexityCheck.java:40:  'NPathComplexityCheck'
gui/CodeSelectorPModel.java:34:  'CodeSelectorPModel'
gui/JTreeTable.java:56:  'JTreeTable'
gui/ParseTreeTablePModel.java:40:  'ParseTreeTablePModel'

In test:

checks/ArrayTypeStyleCheckTest.java:62:  'testCStyle'
checks/blocks/LeftCurlyCheckTest.java:147:  'testNL2'
checks/blocks/LeftCurlyCheckTest.java:196:  'testNL3'
checks/indentation/IndentationCheckTest.java:225:  'forbidCStyle'
checks/metrics/NPathComplexityCheckTest.java:37:  'NPathComplexityCheckTest'
checks/regexp/RegexpCheckTest.java:244:  'testIgnoreCommentsCStyle'
checks/regexp/RegexpCheckTest.java:257:  'testIgnoreCommentsFalseCStyle'
checks/regexp/RegexpCheckTest.java:271:  'testIgnoreCommentsMultipleCStyle'
checks/regexp/RegexpSinglelineJavaCheckTest.java:122:  'testIgnoreCommentsCStyle'
checks/regexp/RegexpSinglelineJavaCheckTest.java:132:  'testIgnoreCommentsFalseCStyle'
checks/regexp/RegexpSinglelineJavaCheckTest.java:143:  'testIgnoreCommentsMultipleCStyle'
checks/whitespace/NoWhitespaceAfterCheckTest.java:204:  'astRBrake'
filters/SuppressWithNearbyCommentFilterTest.java:137:  'testUsingAVariableMessage'
filters/SuppressWithNearbyCommentFilterTest.java:152:  'testUsingAVariableCheckOnNextLine'
filters/SuppressWithNearbyCommentFilterTest.java:165:  'testUsingAVariableCheckOnPreviousLine'
grammars/java8/LambdaTest.java:150:  'testWithFewArgWIthTypeOneLine'
grammars/java8/LambdaTest.java:170:  'testWIthMultilineBody'
gui/CodeSelectorPModelTest.java:35:  'CodeSelectorPModelTest'
gui/ParseTreeTablePModelTest.java:39:  'ParseTreeTablePModelTest'
internal/XDocUtil.java:40:  'XDocUtil'
internal/XDocsPagesTest.java:68:  'XDocsPagesTest'

@rnveach , do think we should enforce 0 for abbreviations ?
Problematic cases: "CComment", "NPath", "PModel".
all cases in tests could be easily extended.
I actually hate "CComment" name - there is not such term in java, we need to change it.

Member

romani commented Jan 12, 2017

if we make no abbreviations in our code we need to resolve following problems, some of them are complicated , may that is why we stick with length of 0:

in main code:

api/FileContents.java:133:  'reportCComment'
api/FileContents.java:164:  'getCComments'
api/FileContents.java:176:  'extractCComment'
api/FileContents.java:319:  'hasIntersectionWithCComment'
checks/AvoidEscapedUnicodeCharactersCheck.java:295:  'isTrailingCComent'
checks/imports/CustomImportOrderCheck.java:787:  'getFirstNDomainsFromIdent'
checks/indentation/AbstractExpressionHandler.java:544:  'checkRParen'
checks/indentation/AbstractExpressionHandler.java:564:  'checkLParen'
checks/indentation/BlockParentHandler.java:136:  'getLCurly'
checks/indentation/BlockParentHandler.java:145:  'getRCurly'
checks/indentation/BlockParentHandler.java:153:  'checkLCurly'
checks/indentation/BlockParentHandler.java:185:  'checkRCurly'
checks/indentation/BlockParentHandler.java:229:  'getRParen'
checks/indentation/BlockParentHandler.java:238:  'getLParen'
checks/metrics/NPathComplexityCheck.java:40:  'NPathComplexityCheck'
gui/CodeSelectorPModel.java:34:  'CodeSelectorPModel'
gui/JTreeTable.java:56:  'JTreeTable'
gui/ParseTreeTablePModel.java:40:  'ParseTreeTablePModel'

In test:

checks/ArrayTypeStyleCheckTest.java:62:  'testCStyle'
checks/blocks/LeftCurlyCheckTest.java:147:  'testNL2'
checks/blocks/LeftCurlyCheckTest.java:196:  'testNL3'
checks/indentation/IndentationCheckTest.java:225:  'forbidCStyle'
checks/metrics/NPathComplexityCheckTest.java:37:  'NPathComplexityCheckTest'
checks/regexp/RegexpCheckTest.java:244:  'testIgnoreCommentsCStyle'
checks/regexp/RegexpCheckTest.java:257:  'testIgnoreCommentsFalseCStyle'
checks/regexp/RegexpCheckTest.java:271:  'testIgnoreCommentsMultipleCStyle'
checks/regexp/RegexpSinglelineJavaCheckTest.java:122:  'testIgnoreCommentsCStyle'
checks/regexp/RegexpSinglelineJavaCheckTest.java:132:  'testIgnoreCommentsFalseCStyle'
checks/regexp/RegexpSinglelineJavaCheckTest.java:143:  'testIgnoreCommentsMultipleCStyle'
checks/whitespace/NoWhitespaceAfterCheckTest.java:204:  'astRBrake'
filters/SuppressWithNearbyCommentFilterTest.java:137:  'testUsingAVariableMessage'
filters/SuppressWithNearbyCommentFilterTest.java:152:  'testUsingAVariableCheckOnNextLine'
filters/SuppressWithNearbyCommentFilterTest.java:165:  'testUsingAVariableCheckOnPreviousLine'
grammars/java8/LambdaTest.java:150:  'testWithFewArgWIthTypeOneLine'
grammars/java8/LambdaTest.java:170:  'testWIthMultilineBody'
gui/CodeSelectorPModelTest.java:35:  'CodeSelectorPModelTest'
gui/ParseTreeTablePModelTest.java:39:  'ParseTreeTablePModelTest'
internal/XDocUtil.java:40:  'XDocUtil'
internal/XDocsPagesTest.java:68:  'XDocsPagesTest'

@rnveach , do think we should enforce 0 for abbreviations ?
Problematic cases: "CComment", "NPath", "PModel".
all cases in tests could be easily extended.
I actually hate "CComment" name - there is not such term in java, we need to change it.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 12, 2017

Member

reportCComment
NPathComplexityCheck
XDocUtil
XDocsPagesTest
JTreeTable
CodeSelectorPModel

Those are complicated as they are one letter or dashed words.
Most of the questionable ones are classes, so we can always allow one for them if we can't come up with something else.

'testWithFewArgWIthTypeOneLine'
'testWIthMultilineBody'

Those are just plain mistakes, not abbreviations. :)

do think we should enforce 0 for abbreviations ?

I do.
On my other projects I also try to enforce this. Reading consecutive letters in camel case might be a little confusing at first and I do not see it as camel case to have consecutive capital letters. When I write code, I prefer full words over abbreviations to make it read more like it is a sentence. If I do have an abbreviation, I write as camel case as if it was a word (Html instead of HTML).

Member

rnveach commented Jan 12, 2017

reportCComment
NPathComplexityCheck
XDocUtil
XDocsPagesTest
JTreeTable
CodeSelectorPModel

Those are complicated as they are one letter or dashed words.
Most of the questionable ones are classes, so we can always allow one for them if we can't come up with something else.

'testWithFewArgWIthTypeOneLine'
'testWIthMultilineBody'

Those are just plain mistakes, not abbreviations. :)

do think we should enforce 0 for abbreviations ?

I do.
On my other projects I also try to enforce this. Reading consecutive letters in camel case might be a little confusing at first and I do not see it as camel case to have consecutive capital letters. When I write code, I prefer full words over abbreviations to make it read more like it is a sentence. If I do have an abbreviation, I write as camel case as if it was a word (Html instead of HTML).

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 13, 2017

Member

yes, do like good Camel case, too.

"XDoc" - "Xdoc"
"JTreeTable" - is it custom class so prefix "J" doe snot make any good. We could change it.
"NPathComplexityCheck" -- could be suppressed it is a name of check, we can not change it.

"xxxxxxPModel" - presentation models, "Pmodel", "PresModel" or ... ?

Member

romani commented Jan 13, 2017

yes, do like good Camel case, too.

"XDoc" - "Xdoc"
"JTreeTable" - is it custom class so prefix "J" doe snot make any good. We could change it.
"NPathComplexityCheck" -- could be suppressed it is a name of check, we can not change it.

"xxxxxxPModel" - presentation models, "Pmodel", "PresModel" or ... ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

CComment -> BlockComment (defined in CommentListener)
CppComment -> SinglelineComment (defined in CommentListener)
xxxxxxPModel -> xxxxxxPresentationModel or xxxxxxPresentation (I am fine with long words, I didn't even know this is what P meant)
I will suppress JTreeTable as this is a Java convention for javax.swing. https://docs.oracle.com/javase/7/docs/api/javax/swing/package-summary.html

Member

rnveach commented Feb 13, 2017

CComment -> BlockComment (defined in CommentListener)
CppComment -> SinglelineComment (defined in CommentListener)
xxxxxxPModel -> xxxxxxPresentationModel or xxxxxxPresentation (I am fine with long words, I didn't even know this is what P meant)
I will suppress JTreeTable as this is a Java convention for javax.swing. https://docs.oracle.com/javase/7/docs/api/javax/swing/package-summary.html

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 15, 2017

Member

CComment -> BlockComment (defined in CommentListener)
CppComment -> SinglelineComment (defined in CommentListener)

Ok to update, if smth is still left.
but not for names of Check.

(I am fine with long words, I didn't even know this is what P meant )

I am ok to update too. We are not going to have too much Presentation models in our code, so abbreviation as "P" does not make sense for us. In proprietary code it helped.

I will suppress JTreeTable as this is a Java convention for javax.swing

That is not our convention. Nothing force us to keep name that similar to java standard. Lets avoid illusions.
Lets avoid suppressions, rename it to what ever is more reasonable.

Member

romani commented Feb 15, 2017

CComment -> BlockComment (defined in CommentListener)
CppComment -> SinglelineComment (defined in CommentListener)

Ok to update, if smth is still left.
but not for names of Check.

(I am fine with long words, I didn't even know this is what P meant )

I am ok to update too. We are not going to have too much Presentation models in our code, so abbreviation as "P" does not make sense for us. In proprietary code it helped.

I will suppress JTreeTable as this is a Java convention for javax.swing

That is not our convention. Nothing force us to keep name that similar to java standard. Lets avoid illusions.
Lets avoid suppressions, rename it to what ever is more reasonable.

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 15, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 17, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Feb 17, 2017

romani added a commit that referenced this issue Feb 17, 2017

@romani romani added the bug label Feb 17, 2017

@romani romani added this to the 7.6 milestone Feb 17, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 17, 2017

Member

fix is merged

Member

romani commented Feb 17, 2017

fix is merged

@romani romani closed this Feb 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment