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

launch_diff: upgraded util to not rely on maven plugin #274

Merged
merged 1 commit into from
Dec 28, 2017

Conversation

rnveach
Copy link
Member

@rnveach rnveach commented Dec 26, 2017

To show how #273 should be done, I updated my own script to not rely on maven anymore.
It uses the pure Checkstyle CLI with no updates to patch-diff-report-tool needed and not extra dependencies.

  • All install commands where changed to package now.
  • github support was removed, should just rely on git.
  • added extra code to prevent switching to master as if it was a different branch.
  • added branch names and commit ids to final report like groovy scripts.

Here is an example run of the differences for the same PR and config:

Maven: http://rveach.no-ip.org/checkstyle/regression/reports/149/
CLI: http://rveach.no-ip.org/checkstyle/regression/reports/150/

Only real difference is counts for number of files being processed overall. Maven was restricting us to Java only file types while CLI pulls in all files.
No changes in difference reporting. No changes to internal web page acting as interface for user to program.
Slight speed increase as we don't need to copy large quantity of files around (twice) or try to minimize those files. Less output to console too (263kb versus 103kb).

An external change will be needed to switch to using CLI full time. Excludes uses a wildcard match while CLI excludes must be a pattern matching. So all excludes will need to be updated in scope of #273 .

@rnveach
Copy link
Member Author

rnveach commented Dec 26, 2017

I am unsure what travis failure is about.

@rnveach rnveach requested a review from romani December 26, 2017 22:11
@romani
Copy link
Member

romani commented Dec 27, 2017

Mi be some network issue, restarted.

@romani
Copy link
Member

romani commented Dec 27, 2017

Ones CIs resolved ok to merge.

@rnveach
Copy link
Member Author

rnveach commented Dec 27, 2017

@romani TC failure is from releasenotes-builder .
Travis is passing now.

@rnveach
Copy link
Member Author

rnveach commented Dec 28, 2017

@romani TC failures look false.
https://teamcity.jetbrains.com/viewLog.html?buildId=1269090&tab=Inspection&buildTypeId=Checkstyle_ContributionIdeaInspectionsPullRequest

Unused import (64)

None of these are true. Eclipse gives me no errors.

Declaration has problems in Javadoc references (9)

All these are exceptions and can be found by the compiler.

Redundant throws clause (Errors) (4)

These exceptions are thrown by the utilities.

@rnveach
Copy link
Member Author

rnveach commented Dec 28, 2017

TC has passed after a re-run.

@rnveach rnveach merged commit b3318e9 into checkstyle:master Dec 28, 2017
@rnveach rnveach deleted the launch_diff_update branch December 28, 2017 00:52
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.

2 participants