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 #10866: Fix false positive for switch expressions with operator in MissingSwitchDefault #10875

Merged
merged 1 commit into from Oct 26, 2021

Conversation

KENNYSOFT
Copy link
Contributor

@KENNYSOFT KENNYSOFT commented Oct 18, 2021

Issue #10866: Fix false positive for switch expressions with operator in MissingSwitchDefault

Diff Regression config: https://gist.githubusercontent.com/KENNYSOFT/617d1d36499254a787df66c050902248/raw/3e2f9ab80feafd78176475ff15b22da50e24e5d6/my_check-MissingSwitchDefault.xml

@KENNYSOFT
Copy link
Contributor Author

KENNYSOFT commented Oct 18, 2021

GitHub, generate report

@strkkk
Copy link
Member

strkkk commented Oct 18, 2021

@KENNYSOFT you need to change your config. You are using template config, but you need to add there check you fixed (MissingSwitchDefault)

... under treewalker node ...
<module name="MissingSwitchDefault "/>

In this case regression will run with your check. The report above is useless and shows nothing.

@KENNYSOFT
Copy link
Contributor Author

KENNYSOFT commented Oct 18, 2021

@KENNYSOFT you need to change your config. You are using template config, but you need to add there check you fixed (MissingSwitchDefault)

... under treewalker node ...
<module name="MissingSwitchDefault "/>

In this case regression will run with your check. The report above is useless and shows nothing.

@strkkk Thanks. Actually I should re-run it because of the force-push. Will do it with your comment soon; after I find the best place for store the permanent config. (Somewhere else from GitHub Gist...)

@strkkk
Copy link
Member

strkkk commented Oct 18, 2021

@KENNYSOFT what is wrong with github gists?

@KENNYSOFT
Copy link
Contributor Author

KENNYSOFT commented Oct 18, 2021

@KENNYSOFT what is wrong with github gists?

@strkkk Nothing wrong, it's my preference. But I've just found that the official guide (from you) recommends GitHub Gist, so I'll make my mind and use that. 😁

@KENNYSOFT
Copy link
Contributor Author

GitHub, generate report

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

One minor

@KENNYSOFT KENNYSOFT force-pushed the missingswitchdefault-parent branch 2 times, most recently from 14b30c3 to fb1f8f7 Compare October 18, 2021 12:25
@github-actions
Copy link
Contributor

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/266dfbf_2021123736/reports/diff/index.html

@KENNYSOFT
Copy link
Contributor Author

Please add the **hacktoberfest-accepted** label 🙏

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

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

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/266dfbf_2021123736/reports/diff/checkstyle/index.html#A1
No violation on:
https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/266dfbf_2021123736/reports/diff/checkstyle/xref/home/runner/work/checkstyle/checkstyle/contribution/checkstyle-tester/repositories/checkstyle/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/java14/InputJava14SwitchExpression.java.html#L151

So the users can add the default, but if it is not there, it will only compile with a complete list. So ignoring all these types of switches is ok.

There is not an example of this for this check as another area was what was flagged. Please add cases like this to this check's test.

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

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

@KENNYSOFT
Copy link
Contributor Author

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/266dfbf_2021123736/reports/diff/checkstyle/index.html#A1 No violation on: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/266dfbf_2021123736/reports/diff/checkstyle/xref/home/runner/work/checkstyle/checkstyle/contribution/checkstyle-tester/repositories/checkstyle/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/java14/InputJava14SwitchExpression.java.html#L151

So the users can add the default, but if it is not there, it will only compile with a complete list. So ignoring all these types of switches is ok.

There is not an example of this for this check as another area was what was flagged. Please add cases like this to this check's test.

@rnveach I'm afraid I've not fully got your intend, maybe because of my poor English. 😭 Could you explain what means "There is not an example of this" and "another area was what was flagged"? I think I provided sufficient examples since I cannot make a non-compilable example. If you mean that we should have some test cases with switch expressions having default cases, that already exist in InputMissingSwitchDefaultCheckSwitchExpressions (howMany4, howMany5).

In addition, add example to documentation. https://checkstyle.org/config_coding.html#MissingSwitchDefault_Examples

Will do it. 🗒️

@rnveach
Copy link
Member

rnveach commented Oct 25, 2021

I think that sentence didn't make sense period. I am sorry. I meant to talk about the code was in an area not connected to this test but is a good example to add regardless.

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/266dfbf_2021123736/reports/diff/checkstyle/xref/home/runner/work/checkstyle/checkstyle/contribution/checkstyle-tester/repositories/checkstyle/src/test/resources-noncompilable/com/puppycrawl/tools/checkstyle/grammar/java14/InputJava14SwitchExpression.java.html#L151
Please just add this as an example to the tests to show that no violation will be printed so it is tied to this check.

@KENNYSOFT KENNYSOFT force-pushed the missingswitchdefault-parent branch 3 times, most recently from 77958a5 to da90716 Compare October 25, 2021 17:19
@KENNYSOFT
Copy link
Contributor Author

KENNYSOFT commented Oct 25, 2021

Cannot see any log from wercker (401 shown; seems to be related to the permission) and have no idea why it failed. 🤔

@nrmancuso
Copy link
Member

nrmancuso commented Oct 25, 2021

wercker failure is related to #10897, wercker has been restarted. Full log at https://app.wercker.com/checkstyle/checkstyle/runs/build/6176ecbc237ec9000884048d?step=6176f105c148e30007abd29e if maintainers are interested.

@KENNYSOFT
Copy link
Contributor Author

wercker failure is related to #10897, wercker has been restarted. Full log at https://app.wercker.com/checkstyle/checkstyle/runs/build/6176ecbc237ec9000884048d?step=6176f105c148e30007abd29e if maintainers are interested.

Alright, so can I ignore that failure or just wait for more re-attempts to succeed?

@nrmancuso
Copy link
Member

Yes, please ignore failure.

@rnveach rnveach merged commit 2e840c6 into checkstyle:master Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants