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

Issue #4364: Design a new format of suppression dtd schema to support XPath queries #4427

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

timurt
Copy link
Contributor

@timurt timurt commented Jun 6, 2017

@codecov-io
Copy link

codecov-io commented Jun 6, 2017

Codecov Report

Merging #4427 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4427   +/-   ##
=======================================
  Coverage   99.99%   99.99%           
=======================================
  Files         285      285           
  Lines       15297    15297           
  Branches     3482     3482           
=======================================
  Hits        15296    15296           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8874c6...6f38c1b. Read the comment docs.

@rnveach
Copy link
Member

rnveach commented Jun 7, 2017

#4364 (comment)

  1. Please add the new element to the suppression dtd schema;
  2. Bump schema version;

I thought we were extending existing schema?
If we make brand new DTD type, than it seems we don't need to rename suppress tag to be suppress-xpath? You are basically double defining it as an xpath. By the DTD declaration and the tag name.

Also, should we really merge this DTD by itself even though we aren't using it for anything yet?
Why don't we wait on a basic code implementation to make sure there are no hidden issues before merging? I don't see how delaying can hurt anything.

@MEZk
Copy link
Contributor

MEZk commented Jun 7, 2017

I thought we were extending existing schema?

  1. The existing suppression dtd schema should be extended. @timurt please update the format of the existing one.

Also, should we really merge this DTD by itself even though we aren't using it for anything yet?

  1. It will not affect any code or SuppressionLoader as the version of the schema will be 1.2. The next step will be Support suppression-xpath element in SuppressionLoader #4421 and Mapper of XPath expressions onto AST nodes #4369. We agreed on the element name and its attributes, that is why I believe that we can add the schema. The format of the new element is pretty similar to suppression element, that is why I think that there will not be any problems/typos.

@romani
Copy link
Member

romani commented Jun 7, 2017

@timurt ,
please read my comment at #4364 (comment) for name of dtd.

<!ELEMENT suppress-xpath EMPTY>
<!ATTLIST suppress-xpath files CDATA #REQUIRED
checks CDATA #IMPLIED
id CDATA #IMPLIED
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need id?

Copy link
Member

Choose a reason for hiding this comment

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

How else are you going to distinguish suppressing a specific check when multiple instances of the check are defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot about the fact that we can have multiple modules configured.

@timurt
Ignore my comment

@rnveach rnveach requested a review from romani June 8, 2017 16:56
@rnveach
Copy link
Member

rnveach commented Jun 8, 2017

Feel free to merge @romani.

@romani romani merged commit 1056372 into checkstyle:master Jun 8, 2017
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.

5 participants