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 #4343: moved more variables inside if blocks to reduce execution #4343

Merged
merged 1 commit into from May 11, 2017

Conversation

Projects
None yet
3 participants
@rnveach
Member

rnveach commented May 10, 2017

Fixes for new check at sevntu-checkstyle/sevntu.checkstyle#565 .

Will rename commit to reference this PR.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 10, 2017

Member

Code coverage is failing at

if (previousSibling.getType() == TokenTypes.SEMI
&& isOnPreviousLineIgnoringComments(comment, previousSibling)) {
.

The missing branch is: previousSibling.getType() == TokenTypes.SEMI being true and isOnPreviousLineIgnoringComments(comment, previousSibling) being false.
I'm not sure I see how to fix it.

Member

rnveach commented May 10, 2017

Code coverage is failing at

if (previousSibling.getType() == TokenTypes.SEMI
&& isOnPreviousLineIgnoringComments(comment, previousSibling)) {
.

The missing branch is: previousSibling.getType() == TokenTypes.SEMI being true and isOnPreviousLineIgnoringComments(comment, previousSibling) being false.
I'm not sure I see how to fix it.

rnveach added a commit to rnveach/checkstyle that referenced this pull request May 10, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 10, 2017

Member

I luckily found one case, and weird one, in regression.

Member

rnveach commented May 10, 2017

I luckily found one case, and weird one, in regression.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach May 11, 2017

Member

Missed the other spot missing coverage. Should be fixed.

Member

rnveach commented May 11, 2017

Missed the other spot missing coverage. Should be fixed.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 11, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4343   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         283     283           
  Lines       15208   15209    +1     
  Branches     3450    3450           
======================================
+ Hits        15208   15209    +1
Impacted Files Coverage Δ
...ckstyle/checks/coding/FinalLocalVariableCheck.java 100% <ø> (ø) ⬆️
...yle/checks/coding/ExplicitInitializationCheck.java 100% <100%> (ø) ⬆️
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø) ⬆️
...e/checks/indentation/CommentsIndentationCheck.java 100% <100%> (ø) ⬆️
...yle/checks/AvoidEscapedUnicodeCharactersCheck.java 100% <100%> (ø) ⬆️
.../checkstyle/checks/javadoc/JavadocMethodCheck.java 100% <100%> (ø) ⬆️
...uppycrawl/tools/checkstyle/utils/JavadocUtils.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 a94a5a7...1de27c4. Read the comment docs.

codecov-io commented May 11, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4343   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         283     283           
  Lines       15208   15209    +1     
  Branches     3450    3450           
======================================
+ Hits        15208   15209    +1
Impacted Files Coverage Δ
...ckstyle/checks/coding/FinalLocalVariableCheck.java 100% <ø> (ø) ⬆️
...yle/checks/coding/ExplicitInitializationCheck.java 100% <100%> (ø) ⬆️
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø) ⬆️
...e/checks/indentation/CommentsIndentationCheck.java 100% <100%> (ø) ⬆️
...yle/checks/AvoidEscapedUnicodeCharactersCheck.java 100% <100%> (ø) ⬆️
.../checkstyle/checks/javadoc/JavadocMethodCheck.java 100% <100%> (ø) ⬆️
...uppycrawl/tools/checkstyle/utils/JavadocUtils.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 a94a5a7...1de27c4. Read the comment docs.

@rnveach

This comment has been minimized.

Show comment
Hide comment

@rnveach rnveach requested a review from romani May 11, 2017

@rnveach rnveach changed the title from minor: moved more variables inside if blocks to reduce execution time to Pull #4343: moved more variables inside if blocks to reduce execution May 11, 2017

@romani romani merged commit 073c370 into checkstyle:master May 11, 2017

8 checks passed

IDEA Inspections Pull Request (Checkstyle) TeamCity build finished
Details
Shippable Run 2869 status is SUCCESS.
Details
codacy/pr Good work! A positive pull request.
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to a94a5a7
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:sevntu_pull_565 branch May 11, 2017

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