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.

Sign up for GitHub

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

Jump to bottom

Issue #13345: Enabled SeverityMatch example tests #15021

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #13345: Enabled SeverityMatch example tests #15021

wants to merge 1 commit into from

Conversation

SteLeo1602
Copy link
Contributor

Issue: #13345

Enabled the example tests for SeverityMatch filter.

@SteLeo1602
Copy link
Contributor Author

GitHub, generate website

@github-actions GitHub Actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/73260d3_2024045058/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/73260d3_2024045058/filters/severitymatchfilter.html#Example1-config

romani
romani requested changes Jun 18, 2024
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

all good,
minor improvemnt:

src/xdocs/filters/severitymatchfilter.xml Outdated
Comment on lines 75 to 77
public void method1(int V1){} // ok, ParameterNameCheck's severity is info

public void Method2(){} // violation, 'must match pattern'
Copy link
Member

Choose a reason for hiding this comment

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

please do us a favor and change Example to:

public class Example1 {
  public void method1(int V1){} // ok, ParameterName's severity is info

  public void Method2(){} // violation, MethodName's severity is default it is error
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I have to change it one by one? If so, how do I make it pass the CI?

Copy link
Member

Choose a reason for hiding this comment

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

Strange, we just changed wording of explanations, can you check if ' in reasons is root of problems?

Copy link
Member

@romani romani Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://regex101.com/r/yNND7u/1
image

Copy link
Member

Choose a reason for hiding this comment

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

we should match to VIOLATION_WITH_EXPLANATION_PATTERN

Copy link
Member

Choose a reason for hiding this comment

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

I pushed extra commit to let you see more clearly what was a problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, squashed the commit

@romani romani mentioned this pull request Jun 23, 2024
Closed
@SteLeo1602 SteLeo1602 force-pushed the EnableSeverityMatch branch 2 times, most recently from 5db5145 to f9a75d1 Compare June 27, 2024 01:38
romani
romani approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok to merge.

Removed violation comments are good as they are at wrong place, they were never active, with fix become active - required to be removed

@romani romani requested a review from rnveach June 28, 2024 04:01
@SteLeo1602
Copy link
Contributor Author

@rnveach I wanted to remind you to review my changes here. The last feedback was over a week ago and I think I should notify you by now.

rnveach
rnveach requested changes Jul 9, 2024
View reviewed changes
src/test/java/com/puppycrawl/tools/checkstyle/bdd/InlineConfigParser.java Outdated
@@ -98,7 +98,7 @@ public final class InlineConfigParser {

/** A pattern to find the string: "// violation, explanation". */
private static final Pattern VIOLATION_WITH_EXPLANATION_PATTERN = Pattern
.compile(".*//\\s*violation,\\s.+\\s(?:['\"](.*)['\"])?$");
.compile(".*//\\s*violation,\\s.+\\s(?:.*)?$");
Copy link
Member

Choose a reason for hiding this comment

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

Why was this changed? No other PRs so far have had an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Nobody noticed defect in this code before until this migration.

Copy link
Member

Choose a reason for hiding this comment

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

What was the defect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before it assumed that there should be quotation marks around the explanation just like it would be around violation's message

Copy link
Member

Choose a reason for hiding this comment

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

It was copy paste mistake, fortunately no much usage of this violation style in use.
This PR is first actual usage of this style.

Copy link
Member

@rnveach rnveach Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

".*//\\s*violation,\\s.+\\s(?:.*)?$" => ".*//\\s*violation,\\s+(?:.*)?$"

The extra .+\\s doesn't really make sense.

Copy link
Member

Choose a reason for hiding this comment

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

moved to #15256

src/test/java/com/puppycrawl/tools/checkstyle/bdd/InlineConfigParser.java Outdated
checkWhetherViolationSpecified(specifyViolationMessage, violationMessage,
violationLineNum);
inputConfigBuilder.addViolation(violationLineNum, violationMessage);
inputConfigBuilder.addViolation(violationLineNum, null);
Copy link
Member

Choose a reason for hiding this comment

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

Same for this code.

...ycrawl/tools/checkstyle/checks/coding/equalshashcode/InputEqualsHashCodeEqualsParameter.java Outdated
@@ -12,15 +12,15 @@ public boolean equals(String o) {
return true;
}
}
public static class TestClass2 { // violation, no correct `equals`
public static class TestClass2 {
Copy link
Member

Choose a reason for hiding this comment

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

This also.

Copy link
Member

Choose a reason for hiding this comment

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

This location of comment is wrong, it was ignored before. With this update it becomes actionable and can not stay at this line

Copy link
Member

Choose a reason for hiding this comment

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

Why was this missed? Is this connected to the regexp change?

Copy link
Member

Choose a reason for hiding this comment

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

Regexp was bad, all such lines were ignored.

Copy link
Member

Choose a reason for hiding this comment

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

If this is an issue we ignored such lines, won't we ignore more lines that don't match the regexp? What about violation with no comma? Or with comma but nothing afterwards?

Why aren't we finding all violation lines, and then checking if they match a message and if they don't then fail with an unrecognized message.

Copy link
Member

Choose a reason for hiding this comment

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

It also sounds like this should be split into 2 commits. 1 on bad regexp, other on severity match which PR is tied to.

Copy link
Member

Choose a reason for hiding this comment

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

@SteLeo1602 , ok, this update becomes confusing for others.

Let's create issue on fact that trailing comments without message but with some reason wording is not working. We will fix only it in scope of this issue, separate PR.

And after merging of such update, we will enable this Example.

Copy link
Member

@rnveach rnveach Jul 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't feel any confusion. It seems like there is more going on here and a more deep investigation needs to be done.

We completely missed a // violation comment, and that can happen again regardless of what we update regular expression to.

Also this PR was definitely missing explanations on what its doing and why.
#15021 (comment)
Never got an explanation on this.

Just cause this is tied to an existing issue, doesn't mean we should make a whole bunch of what appears to be unrelated updates without an explanation. If we find bugs, either make them their own PR, own commit, or tie them to their own issue separate from approved work.

Copy link
Member

Choose a reason for hiding this comment

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

@SteLeo1602
new issue is created.
#15256

please fix only it in separate PR.

...examples/resources/com/puppycrawl/tools/checkstyle/filters/severitymatchfilter/Example1.java Outdated Show resolved Hide resolved
@SteLeo1602 SteLeo1602 force-pushed the EnableSeverityMatch branch 2 times, most recently from 555f800 to 8305282 Compare July 9, 2024 22:31
@romani romani mentioned this pull request Jul 14, 2024
Open
@romani romani mentioned this pull request Aug 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romani romani romani approved these changes

@rnveach rnveach rnveach requested changes

Assignees

@rnveach rnveach

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SteLeo1602 @romani @rnveach