Issue #7064: Add influenceFormat property to SuppressWithPlainTextCommentFilter by stoyanK7 · Pull Request #11884 · checkstyle/checkstyle

stoyanK7 · 2022-07-10T19:05:26Z

Resolves #7064

Changes

Adds influenceFormat property along with commentFormat
Adds 3 tests to cover new property
Updates all other tests with a lot more examples covering the new change
Updates all other tests' inline configurations to include the new property
Updates documentation about the new property(I will provide screenshots of the website once the problem below is discussed)
- Examples of how this prop is used in javadoc and site are not added. I didn't add them yet because I think adding a reference to SuppressWithNearbyCommentFilter's examples might be better. In the end it's the same prop doing the exact same thing. No need to repeat it. Let me know what you think.

Diff Regression config: https://gist.githubusercontent.com/stoyanK7/0d95fd3ec8caa5cae9eb597ac8c78b8d/raw/c60fcc04f28f853f530efeceb598e6b5173d4855/config.xml

stoyanK7 · 2022-07-10T19:06:00Z

Github, generate report

stoyanK7 · 2022-07-10T19:08:52Z

-            else {
-                commentFormat = filter.offCommentFormat;
-            }
+            final Pattern filterCommentFormat = getFilterCommentFormat(filter);


This method extraction is needed due to NCSS line count violation.

stoyanK7 · 2022-07-10T19:09:53Z

        /** Off suppression type. */
        OFF,
+        /** Default suppression type. */
+        DEFAULT,


I decided to go with DEFAULT. Not sure if this is the best choice, let me know what you think.

NEARBY sounds good.

yes, please rename. Nearby or area seem better.

stoyanK7 · 2022-07-10T19:11:44Z

 * </p>
 * <ul>
 * <li>
+ * Property {@code commentFormat} - Specify comment pattern to trigger filter to begin suppression.


The description of this property is the same as the next one. I think we can maybe mention it is tied to influenceFormat or something like that?

it is better to make it different, otherwise it seems like a copy-paste and not clear for user

stoyanK7 · 2022-07-10T19:12:38Z

+    private static final String DEFAULT_INFLUENCE_FORMAT = "0";
+
+    /** Specify comment pattern to trigger filter to begin suppression. */
+    private Pattern commentFormat = Pattern.compile(DEFAULT_COMMENT_FORMAT);


I was thinking naming this defaultCommentFormat since in the enum down below I chose DEFAULT, but this sounds misleading to me.

I think it is better to avoid default. Nearby or area seem better to me.

github-actions · 2022-07-10T19:27:29Z

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

stoyanK7 · 2022-07-15T16:33:45Z

@romani @strkkk @rnveach ping

strkkk · 2022-07-16T15:51:14Z

 * </p>
 * <ul>
 * <li>
+ * Property {@code commentFormat} - Specify comment pattern to trigger filter to begin suppression.


it is better to make it different, otherwise it seems like a copy-paste and not clear for user

strkkk · 2022-07-16T15:52:16Z

+ * Property {@code influenceFormat} - Specify negative/zero/positive value that
+ * defines the number of lines preceding/at/following the suppression comment.
+ * Type is {@code java.lang.String}.
+ * Default value is {@code "0"}.


please add config + code examples for this property

Done! All the points that have thumbs up are done and committed. Only on 2 I have left comment that are not done yet.

strkkk · 2022-07-16T15:53:53Z

+    private static final String DEFAULT_INFLUENCE_FORMAT = "0";
+
+    /** Specify comment pattern to trigger filter to begin suppression. */
+    private Pattern commentFormat = Pattern.compile(DEFAULT_COMMENT_FORMAT);


I think it is better to avoid default. Nearby or area seem better to me.

strkkk · 2022-07-16T15:56:17Z

            suppression = new Suppression(offCommentMatcher.group(0),
                lineNo + 1, offCommentMatcher.start(), SuppressionType.OFF, this);
        }
+        if (commentMatcher.find()) {


commentMatcher and onCommentMatcher look confusing, please rename it. Also, from code it looks like there can be a case when suppression from one comment is overwritten by other. Is there some test case for it?

No, no test case for that yet. I think I should tweak it so that there is no overwrite. Do you think this should be the expected behavior?

strkkk · 2022-07-16T15:56:59Z

        /** Off suppression type. */
        OFF,
+        /** Default suppression type. */
+        DEFAULT,


yes, please rename. Nearby or area seem better.

strkkk · 2022-07-16T15:59:42Z

+                else {
+                    firstLine = lineNo + influence;
+                    lastLine = lineNo;
+                }


what about

firstLine = Math.min(lineNo, lineNo + influence); lastLine = Math.max(lineNo, lineNo + influence);

strkkk · 2022-07-16T16:00:32Z

+         */
+        private Pattern getFilterCommentFormat(SuppressWithPlainTextCommentFilter filter) {
+            final Pattern filterCommentFormat;
+            if (suppressionType == SuppressionType.ON) {


please use switch

strkkk · 2022-07-16T16:03:23Z

+         *
+         * @param format          influence format to parse
+         * @param influenceFormat raw influence format
+         * @param text            text of the suppression


please remove indentation for description here and above

strkkk · 2022-07-16T16:08:41Z

+            if (suppressionType == SuppressionType.DEFAULT) {
+                isInScope = eventLine >= firstLine && eventLine <= lastLine;
+            }
+            return isInScope;


may be just use ternary operator and make it

return suppressionType == SuppressionType.DEFAULT ? eventLine >= firstLine && eventLine <= lastLine : lineNo <= eventLine;

Checkstyle does not allow inline conditionals.

stoyanK7 · 2022-07-17T17:14:07Z

Github, generate site

github-actions · 2022-07-17T17:34:45Z

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

romani · 2022-07-17T21:03:16Z

as proof of implementation we need to restore code/comments of suppression at equalsverifier.

From issue. Please create PR to that project. You can use Snapshot version in their pom.

Can we we start to use this feature for google-checks.xml ? And may be other files

checkstyle/config/suppressions.xml

Line 8 in 04cc6c5

So we can avoid suppression by line number, that cause us a lot of pain.

romani · 2022-07-17T21:36:49Z

The more I look at this the more I become confident that we need for this functional new filter, as current filter is about suppression from-to comment. Existing fields first fine for existing design, but not really for new fields.

But we are talking about "around" comment.

Do you have the same filling ?

stoyanK7 · 2022-07-18T16:04:52Z

The more I look at this the more I become confident that we need for this functional new filter,

Do you mean a completely new filter or just change SuppressWithNearbyCommentCheck's parent to Checker?

Existing fields first fine for existing design, but not really for new fields.
Do you have the same filling ?

Kind of, it didn't require much tweaking to make it work in this case. Mostly copying behavior from SuppressWithNearbyCommentFilter.

SuppressWithNearbyCommentFilter uses internal class Tag
SuppressWithPlainTextCommentFilter uses internal class Suppression

but both have similar purpose. I get what you mean though, same thought crossed my mind - it feels like this check is doing too much now.

From issue. Please create PR to that project. You can use Snapshot version in their pom.

Should I still do it or shall we first discuss what to do with your proposal?

Can we we start to use this feature for google-checks.xml ? And may be other files

I think yes, absolutely. Suppressions can be done inline.

romani · 2022-07-19T03:57:30Z

completely new filter.
Users do not care about filter name, they want easy to configure. Module with numerous properties blow mind of users.

Should I still do it or shall we first discuss what to do with your proposal?

Let's finish discussion on design.

Suppressions can be done inline.

Please do now, to prove that it is good.

Kind of, it didn't require much tweaking

Let's think of user experience, not our efforts.

@strkkk , what is your thoughts on this?

…mentFilter

stoyanK7 · 2022-07-19T15:58:19Z

I completely agree with your statements here.

Added a second commit just to show that the new functionality works.

romani · 2022-07-20T16:19:05Z

@stoyanK7 , if you and me think that new Filter will be more logical and has better design, lets create new issue to describe new Filter and convert this PR to use new filter, I pretty sure we will reuse bunch of code(in some form) from other filters , so it will not be very time consuming.

stoyanK7 · 2022-07-20T16:20:28Z

On it. Raising the issue now.

strkkk · 2022-07-21T05:48:47Z

@strkkk , what is your thoughts on this?

new filter seems fine.

@stoyanK7 I think this PR along with the issue can be closed as work will continue in new issues/PR

stoyanK7 commented Jul 10, 2022

View reviewed changes

strkkk requested changes Jul 16, 2022

View reviewed changes

strkkk self-assigned this Jul 16, 2022

stoyanK7 added 2 commits July 19, 2022 18:54

Issue #7064: Add influenceFormat property to SuppressWithPlainTextCom…

f5321d3

…mentFilter

Issue #7064: Example usage of new functionality

b3a8845

stoyanK7 mentioned this pull request Jul 20, 2022

New filter: SuppressWithNearbyTextFilter to suppress checks with inline comments in any file type #11939

Closed

stoyanK7 closed this Jul 21, 2022

Uh oh!

Conversation

stoyanK7 commented Jul 10, 2022

Changes

Uh oh!

stoyanK7 commented Jul 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoyanK7 Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 10, 2022

Uh oh!

stoyanK7 commented Jul 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoyanK7 Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoyanK7 commented Jul 17, 2022

Uh oh!

github-actions Bot commented Jul 17, 2022

Uh oh!

romani commented Jul 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Jul 17, 2022

Uh oh!

stoyanK7 commented Jul 18, 2022

Uh oh!

romani commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stoyanK7 commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Jul 20, 2022

Uh oh!

stoyanK7 commented Jul 20, 2022

Uh oh!

strkkk commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

stoyanK7 Jul 10, 2022 •

edited

Loading

stoyanK7 Jul 17, 2022 •

edited

Loading

romani commented Jul 17, 2022 •

edited

Loading

romani commented Jul 19, 2022 •

edited

Loading

stoyanK7 commented Jul 19, 2022 •

edited

Loading