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

Make SuppressionCommentFilter and SuppressWithNearbyCommentFilter children of TreeWalker #4714

Closed
MEZk opened this Issue Jul 13, 2017 · 15 comments

Comments

@MEZk
Contributor

MEZk commented Jul 13, 2017

SuppressWithNearbyCommentFilter and SuppressionCommentFilter need to have information which can be extracted from AST. TreeWalker is the only one FileSetCheck which deals with AST. That is why it is reasonable to make both filters children of TreeWalker. AST must not be visible on the Checker level.

What should be done:

  1. Create TreeWalkerAuditEvent. TreeWalkerAuditEvent is a wrapper class with the following set of fields:
private String fileName;
private FileContents fileContents;
private LocalizedMessage localizedMessage;

We should not extend AuditEvent or create AbstractAuditEvent as we cannot design the whole model of events now. Thus, the concrete implementation is OK.

  1. Create AstFilter interface with the following api:
boolean accept(TreeWalkerAuditEvent auditEvent);
  1. SuppressionCommentFilter and SuppressWithNearbyCommentFilter should implement AstFilter. Dependency on FileContentsHolder must be removed. All UTs should be fixed.

  2. Add the set of AstFilters to TreeWalker.

  3. All AstFilters should be initialized and configured in setupChild method of TreeWalker (similar to Checker).

  4. Add the sorted set of localized messages to TreeWalker.

  5. The set of messages should be cleared in notifyBegin method of TreeWalker.

  6. The messages, received from the parents of AbstractCheck should be stored to the sorted set of messages in TreeWalker inside notifyEnd method.

  7. TreeWalker has to filter the set of sorted messages in processFiltered. All messages which pass the filtration should be given to the apper level to Checker (AbstractFileSetCheck#addMessages). TreeWalkerAuditEvent objects need to be created based on LocalizedMessage instances.

@timurt
You can use my branch as a starting point.

==========================

ATTENTION (config migration process):
SuppressWithNearbyCommentFilter - just move config part under TreeWalker (from Checker).
SuppressCommentFilter - Unfortunately we caused regression by this fix till 8.6 version for SuppressCommentFilter, upgrade is to use new Filter http://checkstyle.sourceforge.net/config_filters.html#SuppressWithPlainTextCommentFilter - just rename name of module. SuppressCommentFilter become a bit different Filter by functionality - become java code related only (can not suppress violations in comments anymore).
Example or config update from 8.0 to 8.6 : OpenGamma/OG-Tools@7fc6a37

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 14, 2017

Member

private DetailAST root;

Please introduce this field only when you introduce a filter that will use it, it is a different task.

Member

romani commented Jul 14, 2017

private DetailAST root;

Please introduce this field only when you introduce a filter that will use it, it is a different task.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 14, 2017

Contributor

@romani
Corrected description

Contributor

MEZk commented Jul 14, 2017

@romani
Corrected description

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 15, 2017

Contributor

@timurt
Ping, what is the status of this issue? Do you need help?

Contributor

MEZk commented Jul 15, 2017

@timurt
Ping, what is the status of this issue? Do you need help?

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 16, 2017

Collaborator

@MEZk
I have done 1-8 steps
9. by filtration you mean same as inside Checker#fireErrors?

SuppressionCommentFilter and SuppressWithNearbyCommentFilter can be children for both classes Checker and TreeWalker or only for TreeWalker? should these checks implement Filter interface?

Collaborator

timurt commented Jul 16, 2017

@MEZk
I have done 1-8 steps
9. by filtration you mean same as inside Checker#fireErrors?

SuppressionCommentFilter and SuppressWithNearbyCommentFilter can be children for both classes Checker and TreeWalker or only for TreeWalker? should these checks implement Filter interface?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 16, 2017

Contributor

@timurt
You need to filter the set of LocalizedMessage and return to Checker all messages that passed filtration. No need to work with listeners or cache for now.

Contributor

MEZk commented Jul 16, 2017

@timurt
You need to filter the set of LocalizedMessage and return to Checker all messages that passed filtration. No need to work with listeners or cache for now.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 17, 2017

Contributor

@timurt
Ping

SuppressionCommentFilter and SuppressWithNearbyCommentFilter can be children for both classes Checker and TreeWalker or only for TreeWalker?

No, they should be the children of TreeWalekr only.

should these checks implement Filter interface?

No, they should implement AstFilter interface only.

Contributor

MEZk commented Jul 17, 2017

@timurt
Ping

SuppressionCommentFilter and SuppressWithNearbyCommentFilter can be children for both classes Checker and TreeWalker or only for TreeWalker?

No, they should be the children of TreeWalekr only.

should these checks implement Filter interface?

No, they should implement AstFilter interface only.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Jul 18, 2017

Collaborator

@MEZk
sent PR
Should I change checkstyle_sevntu_checks.xml, move SuppressWithNearbyCommentFilter to TreeWalker module? - I did it inside checkstyle_checks.xml

Collaborator

timurt commented Jul 18, 2017

@MEZk
sent PR
Should I change checkstyle_sevntu_checks.xml, move SuppressWithNearbyCommentFilter to TreeWalker module? - I did it inside checkstyle_checks.xml

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 18, 2017

Contributor

@romani

Should I change checkstyle_sevntu_checks.xml

As far as I know we should release Checkstyle with the changes made in the scope of this issue, change release Checkstyle version which is used by SevNTU Checkstyle, update SevNTU checkstyle version in main project, and change checkstyle_sevntu_checks.xml. Am I right?

Contributor

MEZk commented Jul 18, 2017

@romani

Should I change checkstyle_sevntu_checks.xml

As far as I know we should release Checkstyle with the changes made in the scope of this issue, change release Checkstyle version which is used by SevNTU Checkstyle, update SevNTU checkstyle version in main project, and change checkstyle_sevntu_checks.xml. Am I right?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 18, 2017

Member

Am I right?

yes, this is example of what other thirparty extensions should do to upgrade to latest version of checkstyle. We can not do all at ones, so we will need to disable sevntu for some time, till we release checkstyle 8.1 and next version of sevntu that is based on checkstyle 8.1 .
checkstyle 8.1 will be released at this weekend, so if you prepare PRs for sevntu project we can make upgrade quickly, without bad code leak to master.

Member

romani commented Jul 18, 2017

Am I right?

yes, this is example of what other thirparty extensions should do to upgrade to latest version of checkstyle. We can not do all at ones, so we will need to disable sevntu for some time, till we release checkstyle 8.1 and next version of sevntu that is based on checkstyle 8.1 .
checkstyle 8.1 will be released at this weekend, so if you prepare PRs for sevntu project we can make upgrade quickly, without bad code leak to master.

timurt added a commit to timurt/checkstyle that referenced this issue Jul 18, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker
@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 18, 2017

Member

@romani @MEZk See PR, it is not just sevntu we will be breaking, but probably all of wercker.

Member

rnveach commented Jul 18, 2017

@romani @MEZk See PR, it is not just sevntu we will be breaking, but probably all of wercker.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Jul 18, 2017

Contributor

@rnveach
Answered in PR, waiting for @romani

Contributor

MEZk commented Jul 18, 2017

@rnveach
Answered in PR, waiting for @romani

@MEZk MEZk moved this from To Do to In Progress in Flexible Suppression Model Jul 18, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 20, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker

timurt added a commit to timurt/checkstyle that referenced this issue Jul 21, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker

timurt added a commit to timurt/checkstyle that referenced this issue Jul 21, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 21, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker

timurt added a commit to timurt/checkstyle that referenced this issue Jul 21, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 21, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker

timurt added a commit to timurt/checkstyle that referenced this issue Jul 21, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 23, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 23, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker

timurt added a commit to timurt/checkstyle that referenced this issue Jul 23, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Jul 24, 2017

Issue checkstyle#4714: Make SuppressionCommentFilter and SuppressWith…
…NearbyCommentFilter children of TreeWalker

timurt added a commit to timurt/checkstyle that referenced this issue Jul 24, 2017

romani added a commit that referenced this issue Jul 24, 2017

romani added a commit that referenced this issue Jul 24, 2017

@romani romani added this to the 8.1 milestone Jul 24, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 24, 2017

Member

fix is merged.

release 8.1 will done today or tomorrow.

Member

romani commented Jul 24, 2017

fix is merged.

release 8.1 will done today or tomorrow.

@romani romani closed this Jul 24, 2017

romani added a commit to checkstyle/contribution that referenced this issue Jul 25, 2017

@MEZk MEZk moved this from In Progress to Done in Flexible Suppression Model Jul 26, 2017

@todd-richmond

This comment has been minimized.

Show comment
Hide comment
@todd-richmond

todd-richmond Jul 27, 2017

this broke previously existing xml files so it would be good to add the fix of moving the old blocks to the TreeWalker section to release notes

todd-richmond commented Jul 27, 2017

this broke previously existing xml files so it would be good to add the fix of moving the old blocks to the TreeWalker section to release notes

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 29, 2017

Member

@todd-richmond , thanks for feedback.
Release notes are generated content from list of fixed issues, it not that simple to put in release notes some custom text.
I updated this issue description to attract users attention to how to migrate.

Member

romani commented Jul 29, 2017

@todd-richmond , thanks for feedback.
Release notes are generated content from list of fixed issues, it not that simple to put in release notes some custom text.
I updated this issue description to attract users attention to how to migrate.

CRogers added a commit to CRogers/gradle-baseline that referenced this issue Sep 29, 2017

Configure the intellij checkstyle plugin to use the correct checkstyl…
…e version

Currently, we do not set the CheckStyle-IDEA version, so it defaults to the latest, 8.1. There was a breaking change introduced in 8.1 that makes the baseline checkstyle files invalid (checkstyle/checkstyle#4714). This means you get an error message and the checkstyle plugin no longer works in IDEA. It's also sadness to have the IDE and gradle running different checkstyle versions.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 22, 2017

Member

patch was not removed from wercker for hibernate-search project that result in problem - #5194 (comment) .

Member

romani commented Oct 22, 2017

patch was not removed from wercker for hibernate-search project that result in problem - #5194 (comment) .

romani added a commit that referenced this issue Oct 22, 2017

romani added a commit that referenced this issue Oct 22, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

anton-johansson added a commit to viskan/checkstyle-configuration that referenced this issue Jan 9, 2018

Fix breaking changes for Checkstyle 8.7
Move `SuppressionCommentFilter` to `TreeWalker` (checkstyle/checkstyle#4714).
Remove `FileContentHolder` as it is no longer needed (checkstyle/checkstyle#3573).
Remove `maxLineLength` of `LeftCurlyCheck` as it has been removed (checkstyle/checkstyle#3671).

Closes #7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment