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 3406: Replace Guava's Joiner with Java 8 native approach #3406

Merged
merged 1 commit into from Sep 7, 2016

Conversation

Projects
None yet
3 participants
@MEZk
Contributor

MEZk commented Aug 8, 2016

#3406

One more step to cut down on Checkstyle's dependencies on third-party libraries. The first one was made in #3293

Guava's Joiner was replaced with Java 8 native approach (String#join).

I do not replace usage of Joiner in PackageObjectFactory as it allows to skip nulls. If we use Java 8 approach in order to skip nulls it will require to do the following: http://www.leveluplunch.com/java/examples/stringjoiner-example/#join-list-skipping-null and as far as I know (http://blog.codefx.org/java/stream-performance/) streams performance leaves a lot to be desired.

@romani
Are you OK with such changes?

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

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 8, 2016

Current coverage is 100% (diff: 100%)

Merging #3406 into master will not change coverage

@@           master   #3406   diff @@
=====================================
  Files         276     276          
  Lines       14062   14063     +1   
  Methods         0       0          
  Messages        0       0          
  Branches     3274    3275     +1   
=====================================
+ Hits        14062   14063     +1   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update b4e884c...02e16ce

codecov-io commented Aug 8, 2016

Current coverage is 100% (diff: 100%)

Merging #3406 into master will not change coverage

@@           master   #3406   diff @@
=====================================
  Files         276     276          
  Lines       14062   14063     +1   
  Methods         0       0          
  Messages        0       0          
  Branches     3274    3275     +1   
=====================================
+ Hits        14062   14063     +1   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update b4e884c...02e16ce

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 8, 2016

Member

I do not replace usage of Joiner in PackageObjectFactory as it allows to skip nulls. If we use Java 8 approach in order to skip nulls it will require to do the following

it is ok to use streams there, as this code is executed minimal amount of time during whole validation.
Benefit from reduced guava usage is more desirable.


CI problems looks like temporal CIs failures on CIs sides.

Member

romani commented Aug 8, 2016

I do not replace usage of Joiner in PackageObjectFactory as it allows to skip nulls. If we use Java 8 approach in order to skip nulls it will require to do the following

it is ok to use streams there, as this code is executed minimal amount of time during whole validation.
Benefit from reduced guava usage is more desirable.


CI problems looks like temporal CIs failures on CIs sides.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 8, 2016

Member

It also reasonable to forbid import of guava class that have known jdk8 equavalent, please do.

Member

romani commented Aug 8, 2016

It also reasonable to forbid import of guava class that have known jdk8 equavalent, please do.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 8, 2016

Contributor

@romani
Cobertura cannot generate report in case I use

  return packages.stream().filter(Objects::nonNull)
     .collect(Collectors.joining(className + STRING_SEPARATOR)) + className;

or

  return packages.stream().filter(name -> name != null)
     .collect(Collectors.joining(className + STRING_SEPARATOR)) + className;

We have already had the similar problem (mojohaus/cobertura-maven-plugin#21).
So, I had to use anonymous predicate, however, it violates PMD rule and I think it is a PMD's bug.

        return packages.stream().filter(new Predicate<String>() {
            @Override
            public boolean test(String name) {
                return name != null;
            }
        }).collect(Collectors.joining(className + STRING_SEPARATOR, "", className));
[INFO] PMD Failure: com.puppycrawl.tools.checkstyle.PackageObjectFactory:145 Rule:JUnit4TestShouldUseTestAnnotation Priority:3 JUnit 4 tests that exec
ute tests should use the @Test annotation.
Contributor

MEZk commented Aug 8, 2016

@romani
Cobertura cannot generate report in case I use

  return packages.stream().filter(Objects::nonNull)
     .collect(Collectors.joining(className + STRING_SEPARATOR)) + className;

or

  return packages.stream().filter(name -> name != null)
     .collect(Collectors.joining(className + STRING_SEPARATOR)) + className;

We have already had the similar problem (mojohaus/cobertura-maven-plugin#21).
So, I had to use anonymous predicate, however, it violates PMD rule and I think it is a PMD's bug.

        return packages.stream().filter(new Predicate<String>() {
            @Override
            public boolean test(String name) {
                return name != null;
            }
        }).collect(Collectors.joining(className + STRING_SEPARATOR, "", className));
[INFO] PMD Failure: com.puppycrawl.tools.checkstyle.PackageObjectFactory:145 Rule:JUnit4TestShouldUseTestAnnotation Priority:3 JUnit 4 tests that exec
ute tests should use the @Test annotation.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 8, 2016

Member

This is problem of pmd , please suppress this violation in configuration.

Member

romani commented Aug 8, 2016

This is problem of pmd , please suppress this violation in configuration.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 9, 2016

Member

teamcity is fine now.
wercker is still unstable by connection to github, most likely will be fine as you redo PR.

Member

romani commented Aug 9, 2016

teamcity is fine now.
wercker is still unstable by connection to github, most likely will be fine as you redo PR.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 9, 2016

Contributor

@romani
Do we really need to skip null class names here? If className is null, the NPE will be throw inside createObject, which is invoked inside createObjectWithIgnoringProblems. createObjectWithIgnoringProblems is invoked inside createModule before joinPackageNamesWithClassName.

Contributor

MEZk commented Aug 9, 2016

@romani
Do we really need to skip null class names here? If className is null, the NPE will be throw inside createObject, which is invoked inside createObjectWithIgnoringProblems. createObjectWithIgnoringProblems is invoked inside createModule before joinPackageNamesWithClassName.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 9, 2016

Member

It was done to be more stable on wrong config values. But that is not critical we could keep null values but what is a problem to keep behavior ad it is now ?

Member

romani commented Aug 9, 2016

It was done to be more stable on wrong config values. But that is not critical we could keep null values but what is a problem to keep behavior ad it is now ?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 9, 2016

Contributor
    • 2 new suppressions: one for PMD (JUnit4TestShouldUseTestAnnotation) and one for FindBugs (SIC_INNER_SHOULD_BE_STATIC_ANON).
    • 1 UT for mock PackageObjectFactory object, since now we do not have any UTs to test the case when className is null. We cannot implement UT for non-mock object as the execution flow will stop when we receive NPE from createObject method and we will not reach joinPackageNamesWithClassName. I do not like mock UTs as they brake the model of unit testing.
Contributor

MEZk commented Aug 9, 2016

    • 2 new suppressions: one for PMD (JUnit4TestShouldUseTestAnnotation) and one for FindBugs (SIC_INNER_SHOULD_BE_STATIC_ANON).
    • 1 UT for mock PackageObjectFactory object, since now we do not have any UTs to test the case when className is null. We cannot implement UT for non-mock object as the execution flow will stop when we receive NPE from createObject method and we will not reach joinPackageNamesWithClassName. I do not like mock UTs as they brake the model of unit testing.
@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 13, 2016

Contributor

@romani

  1. Suppressed PMD's violation (JUnit4TestShouldUseTestAnnotation) for PackageObjectFactory class. Made joinPackageNamesWithClassName as static to avoid FindBug's violation (SIC_INNER_SHOULD_BE_STATIC_ANON).
  2. Implemented UT to satisfy coverage threshold. As I sad, I don't like the idea of using reflection in UTs to test private methods, but we don't have other options (see why in my previous comment). Are you OK with the implementation?
Contributor

MEZk commented Aug 13, 2016

@romani

  1. Suppressed PMD's violation (JUnit4TestShouldUseTestAnnotation) for PackageObjectFactory class. Made joinPackageNamesWithClassName as static to avoid FindBug's violation (SIC_INNER_SHOULD_BE_STATIC_ANON).
  2. Implemented UT to satisfy coverage threshold. As I sad, I don't like the idea of using reflection in UTs to test private methods, but we don't have other options (see why in my previous comment). Are you OK with the implementation?

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

@romani romani merged commit 9eafaed into checkstyle:master Sep 7, 2016

7 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codacy/pr Good work! A perfect pull request.
Details
codecov/project 100% (+0.00%) compared to b4e884c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/teamcity Finished TeamCity Build Checkstyle :: IDEA Inspections Pull Request : Inspections total: 0, errors: 0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
wercker/build Wercker pipeline passed
Details
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 7, 2016

Member

@MEZk ,
as we go in minimizing of thirdparty dependencies we need to restrict its usage now.

Please create separate PR and move all general allowances of guava usage https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml#L10 to certain package/class.

As soon as we/you do refactoring to remove some class usage - it should be removed from config too to never let others easily introduce it back.

Member

romani commented Sep 7, 2016

@MEZk ,
as we go in minimizing of thirdparty dependencies we need to restrict its usage now.

Please create separate PR and move all general allowances of guava usage https://github.com/checkstyle/checkstyle/blob/master/config/import-control.xml#L10 to certain package/class.

As soon as we/you do refactoring to remove some class usage - it should be removed from config too to never let others easily introduce it back.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 7, 2016

Member

work will be continued at #3433

Member

romani commented Sep 7, 2016

work will be continued at #3433

@MEZk MEZk deleted the MEZk:pull3406 branch Sep 11, 2016

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

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