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

Pull #3412: Fix ForbidCertainImports config #3412

Merged
merged 1 commit into from Sep 8, 2016

Conversation

Projects
None yet
4 participants
@MEZk
Contributor

MEZk commented Aug 11, 2016

#3412

  1. Usage of java.util.Stack and java.util.Vector was disallowed for the whole project.
  2. Usage of classes from 'com.puppycrawl.tools.checkstyle.checks.*' package was disallowed for 'com.puppycrawl.tools.checkstyle.api' and 'com.puppycrawl.tools.checkstyle.utils' packages.

New violations appeared due to the second restriction. What should we do with them?

main/java/com/puppycrawl/tools/checkstyle/api/DetailNode.java
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl.   22

main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag. 34
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTag.    35
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTagInfo.    36
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTags.   37

test/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtilsTest.java
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl.   36
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTag.    37
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTagInfo.    38
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTags.   39

Violation messages have incorrect structure.
This import should not match '.+\.checks\..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl. 36
Should be:
This import should not match '.+\.checks\..+' pattern, it is forbidden in test/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtilsTest.java.
or
Import 'com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocTags' should not match '.+\.checks\..+' pattern, it is forbidden in test/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtilsTest.java.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 13, 2016

Member

@MEZk

main/java/com/puppycrawl/tools/checkstyle/api/DetailNode.java

This one is only there because of the JavaDoc see. Put the full import in the JavaDoc, like the one below it, and it shouldn't show up anymore.

Member

rnveach commented Aug 13, 2016

@MEZk

main/java/com/puppycrawl/tools/checkstyle/api/DetailNode.java

This one is only there because of the JavaDoc see. Put the full import in the JavaDoc, like the one below it, and it shouldn't show up anymore.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 13, 2016

Current coverage is 100% (diff: 100%)

Merging #3412 into master will not change coverage

@@           master   #3412   diff @@
=====================================
  Files         276     276          
  Lines       14031   14063    +32   
  Methods         0       0          
  Messages        0       0          
  Branches     3241    3275    +34   
=====================================
+ Hits        14031   14063    +32   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update 963f949...48914ac

codecov-io commented Aug 13, 2016

Current coverage is 100% (diff: 100%)

Merging #3412 into master will not change coverage

@@           master   #3412   diff @@
=====================================
  Files         276     276          
  Lines       14031   14063    +32   
  Methods         0       0          
  Messages        0       0          
  Branches     3241    3275    +34   
=====================================
+ Hits        14031   14063    +32   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update 963f949...48914ac

MEZk added a commit to MEZk/checkstyle that referenced this pull request Aug 13, 2016

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 13, 2016

Contributor

@rnveach

This one is only there because of the JavaDoc see. Put the full import in the JavaDoc, like the one below it, and it shouldn't show up anymore.

Done.

Contributor

MEZk commented Aug 13, 2016

@rnveach

This one is only there because of the JavaDoc see. Put the full import in the JavaDoc, like the one below it, and it shouldn't show up anymore.

Done.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 7, 2016

Member

@MEZk ,

JavadocTagInfo.java , JavadocTags, InvalidJavadocTag, JavadocTag

You can add them to suppression with explanation:
will be deprecated as we completely switch to ANTLR parser for javadoc. All of them are required only for old javadoc parsers.

Member

romani commented Sep 7, 2016

@MEZk ,

JavadocTagInfo.java , JavadocTags, InvalidJavadocTag, JavadocTag

You can add them to suppression with explanation:
will be deprecated as we completely switch to ANTLR parser for javadoc. All of them are required only for old javadoc parsers.

MEZk added a commit to MEZk/checkstyle that referenced this pull request Sep 8, 2016

MEZk added a commit to MEZk/checkstyle that referenced this pull request Sep 8, 2016

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Sep 8, 2016

Contributor

@romani
Done. Added JavadocUtils.java and JavadocUtilsTest.java into suppressions. I still think that the violaion message is not correct. See my comment.

Contributor

MEZk commented Sep 8, 2016

@romani
Done. Added JavadocUtils.java and JavadocUtilsTest.java into suppressions. I still think that the violaion message is not correct. See my comment.

MEZk added a commit to MEZk/checkstyle that referenced this pull request Sep 8, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 8, 2016

Member

please squash commits, I am ready to merge.

I still think that the violaion message is not correct.

diff is:
com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl vs test/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtilsTest.java ?

fully qualified name VS relative file name

http://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/coding/ForbidCertainImportsCheck.html
there is nothing about paths.

Please explain your concerns about message.

Member

romani commented Sep 8, 2016

please squash commits, I am ready to merge.

I still think that the violaion message is not correct.

diff is:
com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocNodeImpl vs test/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtilsTest.java ?

fully qualified name VS relative file name

http://sevntu-checkstyle.github.io/sevntu.checkstyle/apidocs/com/github/sevntu/checkstyle/checks/coding/ForbidCertainImportsCheck.html
there is nothing about paths.

Please explain your concerns about message.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Sep 8, 2016

Contributor

@romani

main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java
This import should not match '.+.checks..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag. 34

This message puzzles me. The import (or pattern) is not forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag, its usage is forbidden in main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java. It is vise versa.

I propose:
Import 'com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag' should not match '.+.checks..+' pattern, it is forbidden. 34

Contributor

MEZk commented Sep 8, 2016

@romani

main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java
This import should not match '.+.checks..+' pattern, it is forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag. 34

This message puzzles me. The import (or pattern) is not forbidden in com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag, its usage is forbidden in main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java. It is vise versa.

I propose:
Import 'com.puppycrawl.tools.checkstyle.checks.javadoc.InvalidJavadocTag' should not match '.+.checks..+' pattern, it is forbidden. 34

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 8, 2016

Member

ok, it do make sense - sevntu-checkstyle/sevntu.checkstyle#469

@MEZk , please do squash of commit to let me do final review and merge.

Member

romani commented Sep 8, 2016

ok, it do make sense - sevntu-checkstyle/sevntu.checkstyle#469

@MEZk , please do squash of commit to let me do final review and merge.

Pull #3412: Disallow usage of java.util.Stack, java.util.Vector in co…
…de, 'checks' package in 'api' and 'utils' packages
@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Sep 8, 2016

Contributor

@romani
Done.
I'll restart the build after #3436 to let you perform the merge.

Contributor

MEZk commented Sep 8, 2016

@romani
Done.
I'll restart the build after #3436 to let you perform the merge.

@romani romani merged commit d563f00 into checkstyle:master Sep 8, 2016

4 of 6 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! A positive pull request.
Details
wercker/build Wercker pipeline passed
Details

@romani romani added this to the 7.2 milestone Sep 8, 2016

@MEZk MEZk deleted the MEZk:pull3412-ForbidCertainImports branch Sep 11, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 20, 2016

Member

@MEZk , please rebase your commit , we need to make all CIs green.

Member

romani commented Sep 20, 2016

@MEZk , please rebase your commit , we need to make all CIs green.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Sep 20, 2016

Contributor

@romani
It is already in master.

Contributor

MEZk commented Sep 20, 2016

@romani
It is already in master.

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