-
Notifications
You must be signed in to change notification settings - Fork 782
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
chore: improve checkstyle double equals test regex & types #1457
Conversation
If this change fixes a false positive, why does the lint script not show so in the PR checks? |
@jthegedus If I am understanding you correctly, the PR checks don't change because no existing code in main is matched by the old regular expression. Code from #1456 does match though (in particular, the |
I thought the Workflows executed from the forked code, not what was on the target branch. https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks Which would lead me to think that this change hasn't matched any previous false positives. |
@jthegedus I don't think we are in disagreement - this change doesn't match any previous false positives, but it will address potential future potential ones ones. Specifically, the false positives are shown in the CI errors of #1456, which yes are executed from the forked code. I think we agree that these changes will fix those false positives? Maybe it's weird that I introduced this fix separately instead of bundling it together with #1456? I'm not sure |
@hyperupcall I think I understand. As you say the false positive wasn't present in the codebase (since the wrong regex in this @jthegedus you can validate this by reverting the change to line 125/old line 118 in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code seems fine but it's not clear to me why this check is needed at all.
The description states: Although it is valid Bash, it reduces consistency and copy-paste-ability
. Is that all there is too it?
@Stratus3D yes, that's it! The check was originally added to help address #1313. Maybe the error message should change to be more descriptive? |
Much better error msg, thanks! |
Summary
General fixes for
checkstyle.py
, including a false positive fix for theno-test-double-equals
rule.