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 #5759: Update AbstractChecks to log DetailAST (part 1) #6221

Closed
wants to merge 1 commit into from

Conversation

pbludov
Copy link
Member

@pbludov pbludov commented Nov 18, 2018

Issue #5759

Regression tests:
https://pbludov.github.io/issue-5759/

Note that patch version reports a bit more violations, for example:
https://pbludov.github.io/issue-5759/checkstyle/index.html#A1151

@rnveach
Copy link
Member

rnveach commented Nov 18, 2018

@romani Do we require xpath tests for every check in this issue?

@romani
Copy link
Member

romani commented Nov 18, 2018

Yes, we demanded this before, we need to keep doing it, as it is the only reason to have such change.

@rnveach
Copy link
Member

rnveach commented Nov 18, 2018

@pbludov See #6207 for what tests we are asking for for the checks changing in this PR.

@pbludov pbludov changed the title Issue #5759: Update AbstractChecks to log DetailAST (part 1) [WIP] Issue #5759: Update AbstractChecks to log DetailAST (part 1) Nov 19, 2018
@pbludov pbludov force-pushed the issue-5759 branch 2 times, most recently from 46cfb63 to 68bf3d0 Compare November 22, 2018 20:40
@pbludov pbludov changed the title [WIP] Issue #5759: Update AbstractChecks to log DetailAST (part 1) [WIP] I\ssue #5759: Update AbstractChecks to log DetailAST (part 1) Nov 23, 2018
@pbludov pbludov changed the title [WIP] I\ssue #5759: Update AbstractChecks to log DetailAST (part 1) Issue #5759: Update AbstractChecks to log DetailAST (part 1) Nov 23, 2018
@pbludov
Copy link
Member Author

pbludov commented Nov 23, 2018

@romani Do we require xpath tests for every check in this issue?

done.

@romani
Copy link
Member

romani commented Nov 24, 2018

@pbludov , thanks a lot for update.
68 files changed ...... it will force me to find a lot time to throughly review it.

Is it possible to split this PR in few ? by Check or by package to make review easier. There are changes in test expectations that I would like to attentively review.

"20:38: " + getCheckMessage(messages, msgPreceded, "<"),
"20:40: " + getCheckMessage(messages, msgFollowed, "<"),
"20:61: " + getCheckMessage(messages, msgPreceded, ">"),
"12:17: " + getCheckMessage(messages, msgFollowed, "<"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 is correct, verified at InputWhitespaceAroundGenerics, symbol < is at 17 column.

@romani
Copy link
Member

romani commented Nov 30, 2018

@pbludov , verification of such big update is very complicated.
do you have time to split it in several parts ?

@pbludov pbludov closed this Dec 9, 2018
@pbludov pbludov deleted the issue-5759 branch December 20, 2018 21:14
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