Skip to content
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

Issue #3497: Split Guard class #3510

Merged
merged 1 commit into from Oct 23, 2016
Merged

Conversation

jochenvdv
Copy link
Contributor

@jochenvdv jochenvdv commented Oct 22, 2016

Issue #3497

This is a work in progress.

I have decided to use an abstract base class anyway, because I feel without one the code wasn't clean.
For your convenience I made all changes in logically separated commits (will squash later).
All unit tests, coverage and style checks pass locally.
Please let me know if anything should be done differently (I am only just learning Java).

Things that possibly still need to be done (should this maybe be a separate PR?):

  • Rename PkgControl to ImportControl
  • Refactor tests for PkgControl:
    • Rename and reorganize test files (are separate files really needed for one class being tested?)
    • Add tests that use ClassImportRule (it is covered by already by ImportControlCheckTest though)
  • Double-check if all documentation is using the new names

@codecov-io
Copy link

codecov-io commented Oct 22, 2016

Current coverage is 100% (diff: 100%)

Merging #3510 into master will not change coverage

@@           master   #3510   diff @@
=====================================
  Files         271     273     +2   
  Lines       13079   13074     -5   
  Methods         0       0          
  Messages        0       0          
  Branches     2979    2978     -1   
=====================================
- Hits        13079   13074     -5   
  Misses          0       0          
  Partials        0       0          

Powered by Codecov. Last update 0e7beca...28de3c7

@jochenvdv jochenvdv closed this Oct 22, 2016
@jochenvdv jochenvdv reopened this Oct 22, 2016
@romani
Copy link
Member

romani commented Oct 22, 2016

thanks a lot, code looks good, please squash all in one commit and I will merge it.

@romani
Copy link
Member

romani commented Oct 22, 2016

Rename PkgControl to ImportControl

ok to change. could be in separate PR if that is easy for you.

are separate files really needed for one class being tested?

We prefer testing by functional tests (base on real input files + real config). We use pure UTs only if that is close to impossible to do that by real input file and config.

Refactor tests for PkgControl

please provide proposals.

Double-check if all documentation is using the new names

Classes that you change are not public and visible to user. Only Check properties are public, all other internal classes are not visible to user. All our documentation is located at javadocs and files for maven site - https://github.com/checkstyle/checkstyle/tree/master/src/xdocs

@jochenvdv
Copy link
Contributor Author

jochenvdv commented Oct 23, 2016

@romani I have squashed my commits. It seems the CircleCI build failed because of something unrelated. I rebased onto master and triggered the build again, but it failed again.

I decided to keep this PR as is and maybe do the other stuff later in a separate PR.

@romani
Copy link
Member

romani commented Oct 23, 2016

yes, problem is related to our recent changes, I did some changes in configuration and relaunched your build, I will see how it finish.

@romani romani merged commit e9b3b9c into checkstyle:master Oct 23, 2016
@romani
Copy link
Member

romani commented Oct 23, 2016

CI is passed, thanks a lot for contribution.
For other changes please create separate issue(s) or send PRs with commit that have in message prefix "minor:"

@jochenvdv jochenvdv deleted the split-guard-class branch October 25, 2016 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants