Issue #11315: Add MatchXpath rule to forbid try-catch-fail and refactor utils/xpath packages#19168
Conversation
993efac to
c432b49
Compare
|
hi @romani , the fails are unrelated, please let me know if u need anything else changed |
| <property name="tabWidth" value="4"/> | ||
| <module name="MatchXpath"> | ||
| <property name="query" value="//METHOD_CALL[./IDENT[@text='assertWithMessage'] | ||
| and (./following-sibling::IDENT[@text='fail'])]"/> |
There was a problem hiding this comment.
Can we archive this by usage of forbiddenapi tool ?
https://github.com/checkstyle/checkstyle/blob/master/config/signatures.txt
There was a problem hiding this comment.
@romani i checked the forbiddenapis approach but this is a test only dependency, the rule has to go in signatures-test.txt but as there are still around 240 files using the old assertWithMessage(...).fail() , adding the rule right now breaks the build for other, unmigrated files. and forbiddenapis doesnt allow us to easily suppress the rule for those 240 files while we work on them in smaller prs in batches, as i proposed initially.
how would u want me to work here, i mean we could - maybe migrate all 240 files in this single pr to use signatures-test.txt right now, but it could be a mess, or keep the match-xpath + suppressions.xml approach for this pr and next all batches, then transition it to signatures-test.txt in the final pr.
please let me know which what do u prefer or anyother way we could look at
|
|
||
| <module name="SuppressionSingleFilter"> | ||
| <property name="checks" value="MatchXpath"/> | ||
| <property name="files" value=".*[\\/]sevntu\.checkstyle[\\/].*"/> |
There was a problem hiding this comment.
Please do this suppression in seventh project.
We can merge such update before this PR
There was a problem hiding this comment.
i have opened the pr to add the suppression in the sevntu repo - sevntu-checkstyle/sevntu.checkstyle#1163
let me know when that is merged, then i'll remove the SuppressionSingleFilter from this pr and push for final review
| <suppress id="checkASCII" | ||
| files="[\\/]src[\\/]test[\\/]java[\\/]com[\\/]puppycrawl[\\/]tools[\\/]checkstyle[\\/]ConfigurationLoaderTest.java"/> | ||
|
|
||
| <!-- Remove suppression once all violations are fixed, tracked in #11315 --> |
There was a problem hiding this comment.
Please create new issue to do long lasting migration from one format to another.
It will be one of good xxxx issues.
This PR will fix 11315 . We need your time and efforts to be spent on more complicated issues.
…and refactor utils/xpath packages
c432b49 to
42847df
Compare
|
@romani ig it's ready for final review. ->added a tracking suppression for #19176 in suppressions.xml using the regex (?!utils|xpath) to keep the build green while blocking those regressions. ->removed the SuppressionSingleFilter from this pr's checkstyle-checks.xml. opened a pr directly in the sevntu repo (sevntu-checkstyle/sevntu.checkstyle#1163). ->refactored the migration for the remaining leftover files in the utils and xpath packages. let me know if any other change is needed |
issue #11315
this pr starts fixing the issue described in #11315 by replacing the old
assertWithMessage(...).fail()pattern in try-catch blocks with the cleanergetExpectedThrowableapproach. I have added aMatchXpathrule and fixed all violations in the utils and xpath test . a temporary suppression has been added to keep the build passing while the remaining files are still being migrated. as they all together are close to 240 files in number i plan to fix the rest in smaller follow-up prs and will remove the suppression in the final one