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

Pull #5321: fixed bug on matching xpath when no xpath given #5321

Merged
merged 1 commit into from Dec 5, 2017

Conversation

Projects
None yet
3 participants
@rnveach
Member

rnveach commented Dec 5, 2017

Identified at #5236 (comment) ,

The constructor takes all the same fields as SuppressElement, but it also takes an optional xpathQuery.
For other elements, it is considered matching if the optional parameter is null.

However as of right now, xpathQuery it is considered not a match if is not given.
This seems wrong. An optional parameter should automatically be a match if it is not given. If anything is given without xpathQuery it should act the exact same as a SuppressElement as they are identical.

@rnveach rnveach requested a review from romani Dec 5, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 5, 2017

Member

As CI pass, ok to merge

Member

romani commented Dec 5, 2017

As CI pass, ok to merge

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 5, 2017

Codecov Report

Merging #5321 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5321   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16159   16160    +1     
  Branches     3686    3686           
======================================
+ Hits        16159   16160    +1
Impacted Files Coverage Δ
...ppycrawl/tools/checkstyle/filters/XpathFilter.java 100% <100%> (ø) ⬆️

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 7a30bb9...2b43272. Read the comment docs.

codecov-io commented Dec 5, 2017

Codecov Report

Merging #5321 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #5321   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16159   16160    +1     
  Branches     3686    3686           
======================================
+ Hits        16159   16160    +1
Impacted Files Coverage Δ
...ppycrawl/tools/checkstyle/filters/XpathFilter.java 100% <100%> (ø) ⬆️

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 7a30bb9...2b43272. Read the comment docs.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 5, 2017

Member

CI has passed, I am merging.

Member

rnveach commented Dec 5, 2017

CI has passed, I am merging.

@rnveach rnveach merged commit 799b25c into checkstyle:master Dec 5, 2017

9 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 5667 status is SUCCESS.
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 7a30bb9
Details
continuous-integration/Distelli
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
wercker/build Wercker pipeline passed
Details

@rnveach rnveach deleted the rnveach:bug_in_xpath branch Dec 5, 2017

@romani romani added this to the 8.6 milestone Dec 6, 2017

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

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