Skip to content

Issue #13809: Kill mutation in SuppressWithPlainTextCommentFilter#13888

Merged
romani merged 1 commit intocheckstyle:masterfrom
Kevin222004:suppText
Oct 21, 2023
Merged

Issue #13809: Kill mutation in SuppressWithPlainTextCommentFilter#13888
romani merged 1 commit intocheckstyle:masterfrom
Kevin222004:suppText

Conversation

@Kevin222004
Copy link
Copy Markdown
Contributor

Issue #13809: Kill mutation in SuppressWithPlainTextCommentFilter


report:- https://kevin222004.github.io/PitestReport/com.puppycrawl.tools.checkstyle.filters/SuppressWithPlainTextCommentFilter.java.html



I can see no usage of this both field outside of Suppression class or any such way that they are getting used so Removal will not cause any issue

Comment on lines -313 to -318
/** Suppression text.*/
private final String text;
/** Suppression line.*/
private final int lineNo;
/** Suppression column number.*/
private final int columnNo;
Copy link
Copy Markdown
Member

@Vyom-Yadav Vyom-Yadav Oct 18, 2023

Choose a reason for hiding this comment

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

I don't care about the mutation, but this will flaw the Suppression class and might introduce bugs in the future. Two mutations on the same line in different columns can now be possibly the same.

@romani @rdiachenko Please share your opinion on this.

Copy link
Copy Markdown
Member

@romani romani Oct 19, 2023

Choose a reason for hiding this comment

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

https://checkstyle.org/filters/suppresswithplaintextcommentfilter.html
Suppression is a line comment, usually.
In initial design we did not evy considered column based comments ( when off comment and on comment is on same line)

In our examples we never even tried to use suppression on/off comments to be granulated to columns.
We just try match a line where OFF comment and ON comment, all violations between will be suppressed.

We do use only lines:

private boolean isInScopeOfSuppression(AuditEvent event) {
            return lineNo <= event.getLine();
        }

private boolean isInScopeOfSuppression(AuditEvent event) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In that case, we can remove these fields. Resolved.

Copy link
Copy Markdown
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 when ci is green

@romani
Copy link
Copy Markdown
Member

romani commented Oct 19, 2023

@Kevin222004 , please fix link check validation

Copy link
Copy Markdown
Member

@Vyom-Yadav Vyom-Yadav left a comment

Choose a reason for hiding this comment

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

Please fix CI errors, lgtm!

Copy link
Copy Markdown
Member

@rdiachenko rdiachenko left a comment

Choose a reason for hiding this comment

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

lgtm

@rdiachenko
Copy link
Copy Markdown
Member

rebased

@rdiachenko rdiachenko assigned romani and unassigned rdiachenko Oct 21, 2023
@romani romani merged commit 389df76 into checkstyle:master Oct 21, 2023
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.

4 participants