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

Negate SuppressElement.accept #4979

Closed
rnveach opened this Issue Aug 21, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@rnveach
Copy link
Member

rnveach commented Aug 21, 2017

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/filters/SuppressElement.java#L109-L137
There is some confusion about this code from #4923 (comment) as in a new filter we are rewriting it as a positive.

I think we should do the same in SuppressElement.

I think isLineAndColumnMatch in SuppressElement is actually named incorrectly and should be named isLineAndColumnNotMatch as it returns false in

assertFalse("In range 1-10", filter1.accept(ev));
which is a match.

The final code in accept should be like !isFileNameAndModuleMatching(event) || !isLineAndColumnMatch(event);.

It is also weird that some filters are coded as a negative of others, but I am not sure if we can do something about them.

@romani Please review as these NOTs are starting to confuse me.

@rnveach

This comment has been minimized.

Copy link
Member Author

rnveach commented Feb 19, 2019

@romani

FilterSet is true until a SubFilter returns false
CsvFilter is false until a IntFilter returns true

I assume FilterSet must stay as it is because it is API? CsvFilterElement is ok to change to mimic FilterSet since it is private?

romani added a commit that referenced this issue Feb 19, 2019

@rnveach

This comment has been minimized.

Copy link
Member Author

rnveach commented Feb 20, 2019

FilterSet is true until a SubFilter returns false
CsvFilter is false until a IntFilter returns true

I am going to close this issue as these last item can't be changed.

CsvFilter will not accept anything unless there is 1 sub-filter that is accepted. To invert the logic, we would also have to ensure if all filters aren't accepted that the result isn't accepted either. This won't match FilterSet so there isn't any reason to try.

@rnveach rnveach closed this Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.