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

Support suppression-xpath element in SuppressionLoader #4421

Closed
MEZk opened this Issue Jun 5, 2017 · 16 comments

Comments

4 participants
@MEZk
Contributor

MEZk commented Jun 5, 2017

SuppressionLoader should be able to parse suppression-xpath element and its attributes in accordance with #4364. In the scope of this issue it is required to create basic implementation.

This issue is blocked by #4422.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 12, 2017

Contributor

@timurt
Part 1

  1. Add the set of TreeWalkerFilters to SuppressionLoader
  2. Extract this code to a method.
  3. Add another if block to check suppress-xpath.
  4. Add a method which will return XpathFilter based on the information taken from suppress-xpath attributes.
  5. Add XpathFilter to the set.
  6. Add a new method to load a set of TreeWalkerFilters from xml.

Part 2

  1. Add a new class SuppressionXpathFilter (similar to SuppressionFilter). The filter should implement TreeWalkerFilter, and ExternalResourceHolder interfaces. The filter should also extend AutomaticBean.
  2. Override finishLocalSetup to load the set of XpathFilter objects via SuppressionLoader.
Contributor

MEZk commented Aug 12, 2017

@timurt
Part 1

  1. Add the set of TreeWalkerFilters to SuppressionLoader
  2. Extract this code to a method.
  3. Add another if block to check suppress-xpath.
  4. Add a method which will return XpathFilter based on the information taken from suppress-xpath attributes.
  5. Add XpathFilter to the set.
  6. Add a new method to load a set of TreeWalkerFilters from xml.

Part 2

  1. Add a new class SuppressionXpathFilter (similar to SuppressionFilter). The filter should implement TreeWalkerFilter, and ExternalResourceHolder interfaces. The filter should also extend AutomaticBean.
  2. Override finishLocalSetup to load the set of XpathFilter objects via SuppressionLoader.

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

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 16, 2017

Contributor

@timurt
Any progress?

Contributor

MEZk commented Aug 16, 2017

@timurt
Any progress?

timurt added a commit to timurt/checkstyle that referenced this issue Aug 17, 2017

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Aug 17, 2017

Collaborator

@MEZk
Yes, my commit here

Collaborator

timurt commented Aug 17, 2017

@MEZk
Yes, my commit here

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Aug 17, 2017

Contributor

@timurt
Changes look OK. We are waiting for #4923 .

Start Working on part 2. When we finish #4923 we will merge two parts as separate PR.

Contributor

MEZk commented Aug 17, 2017

@timurt
Changes look OK. We are waiting for #4923 .

Start Working on part 2. When we finish #4923 we will merge two parts as separate PR.

timurt added a commit to timurt/checkstyle that referenced this issue Aug 17, 2017

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

timurt added a commit to timurt/checkstyle that referenced this issue Aug 27, 2017

@MEZk MEZk added the approved label Aug 27, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 30, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 30, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 31, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 31, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 31, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Aug 31, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 2, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 4, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 5, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 10, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 14, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 14, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 15, 2017

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

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

timurt added a commit to timurt/checkstyle that referenced this issue Sep 22, 2017

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

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

timurt added a commit to timurt/checkstyle that referenced this issue Sep 27, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Sep 27, 2017

romani added a commit that referenced this issue Sep 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 28, 2017

Member

issue is not closed as it is referenced in code as "till ...".

Member

romani commented Sep 28, 2017

issue is not closed as it is referenced in code as "till ...".

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 14, 2017

Member

@MEZk @timurt @romani

issue is not closed as it is referenced in code as "till ...".

//Issue: https://github.com/checkstyle/checkstyle/issues/4421

What is required to remove this and complete the issue? I see nothing in this issue/PR stating what we need to do. Are we waiting on another Issue?
PR Notes: #4992 (comment)

Member

rnveach commented Nov 14, 2017

@MEZk @timurt @romani

issue is not closed as it is referenced in code as "till ...".

//Issue: https://github.com/checkstyle/checkstyle/issues/4421

What is required to remove this and complete the issue? I see nothing in this issue/PR stating what we need to do. Are we waiting on another Issue?
PR Notes: #4992 (comment)

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 7, 2017

Contributor

@rnveach @romani
Issue is not finished. We cannot remove the comment.

  1. We need to add the documentation for the new filter. The documentation should explain what checks we support now and the limitations of xpath filter.
  2. We need to use SuppressionXpathFilter in our config.
  3. We need to replace the suppressions in out suppressions.xml with suppress-xpath elements where it is possible.

@timurt
Are you going to continue the work?

Contributor

MEZk commented Dec 7, 2017

@rnveach @romani
Issue is not finished. We cannot remove the comment.

  1. We need to add the documentation for the new filter. The documentation should explain what checks we support now and the limitations of xpath filter.
  2. We need to use SuppressionXpathFilter in our config.
  3. We need to replace the suppressions in out suppressions.xml with suppress-xpath elements where it is possible.

@timurt
Are you going to continue the work?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 8, 2017

Member
  1. We need to replace the suppressions in out suppressions.xml with suppress-xpath elements where it is possible.

This is already another issue, #4991.

Member

rnveach commented Dec 8, 2017

  1. We need to replace the suppressions in out suppressions.xml with suppress-xpath elements where it is possible.

This is already another issue, #4991.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 8, 2017

Contributor

@rnveach

This is already another issue, #4991.

Rigth.

@romani
If I do not receive response from @timurt in a few days, I will continue the work on my own. Is it OK?

Contributor

MEZk commented Dec 8, 2017

@rnveach

This is already another issue, #4991.

Rigth.

@romani
If I do not receive response from @timurt in a few days, I will continue the work on my own. Is it OK?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 15, 2017

Member

Sure, thanks a lot.

Member

romani commented Dec 15, 2017

Sure, thanks a lot.

@timurt

This comment has been minimized.

Show comment
Hide comment
@timurt

timurt Dec 26, 2017

Collaborator

@romani @rnveach @MEZk
Sorry for long delay, I created PR, could you please review

Collaborator

timurt commented Dec 26, 2017

@romani @rnveach @MEZk
Sorry for long delay, I created PR, could you please review

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

rnveach added a commit that referenced this issue Dec 27, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 27, 2017

Member

@MEZk @timurt Confirm everything is done for this issue.

  1. We need to replace the suppressions in out suppressions.xml with suppress-xpath elements where it is possible.

This is already another issue, #4991.

There were some changes for this in the last PR. Is Issue #4991 now not needed?

Member

rnveach commented Dec 27, 2017

@MEZk @timurt Confirm everything is done for this issue.

  1. We need to replace the suppressions in out suppressions.xml with suppress-xpath elements where it is possible.

This is already another issue, #4991.

There were some changes for this in the last PR. Is Issue #4991 now not needed?

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 28, 2017

Contributor

@timurt
https://github.com/checkstyle/checkstyle/pull/5390/files is a great update. Thanks.

@romani @timurt
Why did you decided to have separate file (suppressions-xpath.xml)? The basic idea is to allow the users to have the suppression declarations for both old and new model in one file. The dtd schema allows to do this. Did you do this to show the migration process to avoid the mess in one suppression file?

@rnveach
Yes, #4991 was closed. We will convert other suppressions when we deal with

  1. #4530 (will ease the migration process)
  2. #4830 (will cover all other Checks).
Contributor

MEZk commented Dec 28, 2017

@timurt
https://github.com/checkstyle/checkstyle/pull/5390/files is a great update. Thanks.

@romani @timurt
Why did you decided to have separate file (suppressions-xpath.xml)? The basic idea is to allow the users to have the suppression declarations for both old and new model in one file. The dtd schema allows to do this. Did you do this to show the migration process to avoid the mess in one suppression file?

@rnveach
Yes, #4991 was closed. We will convert other suppressions when we deal with

  1. #4530 (will ease the migration process)
  2. #4830 (will cover all other Checks).
@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Dec 28, 2017

Contributor

@rnveach @romani @timurt
I'm closing this issue. The next steps should be addressed in #4530 and #4830.

Contributor

MEZk commented Dec 28, 2017

@rnveach @romani @timurt
I'm closing this issue. The next steps should be addressed in #4530 and #4830.

@MEZk MEZk closed this Dec 28, 2017

@MEZk MEZk moved this from In Progress to Done in Flexible Suppression Model Dec 28, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 28, 2017

Member

Why did you decided to have separate file (suppressions-xpath.xml)?
#4991 was closed. We will convert other suppressions when we deal with

@MEZk please expand all issues so it is clear on the specifics on what is supposed to be done in each issue so there is no more misunderstandings.

Member

rnveach commented Dec 28, 2017

Why did you decided to have separate file (suppressions-xpath.xml)?
#4991 was closed. We will convert other suppressions when we deal with

@MEZk please expand all issues so it is clear on the specifics on what is supposed to be done in each issue so there is no more misunderstandings.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 29, 2017

Member

Why did you decided to have separate file (suppressions-xpath.xml)?

Looks like lost a context of steps to took to make xpath completed. But it looked so natural, so it did not make rad flag for me during review.

Did you do this to show the migration process to avoid the mess in one suppression file?

lets keep this file separate till complete xpath implementation. The very last update should simple merge of both suppressions in one file.

Member

romani commented Dec 29, 2017

Why did you decided to have separate file (suppressions-xpath.xml)?

Looks like lost a context of steps to took to make xpath completed. But it looked so natural, so it did not make rad flag for me during review.

Did you do this to show the migration process to avoid the mess in one suppression file?

lets keep this file separate till complete xpath implementation. The very last update should simple merge of both suppressions in one file.

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