-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Move coding-style check to GitHub actions #16266
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
base: develop
Are you sure you want to change the base?
Conversation
5497399 to
8059962
Compare
|
Note that |
8059962 to
a28d095
Compare
449afb6 to
efa9895
Compare
|
The main scenario is when a file is modified but the style error is on a line that's not part of the diff hunk. GitHub's For example, if a PR modifies lines 10-20 of a file, but there's a pre-existing style error on line 500, the API will reject the comment with a 422 error because line 500 isn't in the diff. This could happen if:
Since the codebase should be clean as you noted, this is an edge case, but the try/catch prevents a noisy failure when it does happen. |
Thanks for the clarification! While these should be rare given our CI enforcement, I agree that it is better to be defensive here, because changes in the code style script can also change what will be detected. So, I've added the try-catch ;) |
A test triggering the validation error (422) when there is no try-catch can be seen here: https://github.com/r0qs/solidity/actions/runs/20467384783/job/58814376600?pr=14 |
This comment was marked as resolved.
This comment was marked as resolved.
|
@heathdutton motivated me to improve this PR further. I've added error descriptions to our code style checker and pushed the bc25670 with intentional style errors for easier review of the behavior (the file will be removed before merge). That said, I think we should rethink about moving to clang-format as proposed in #2856 for more robust style checking. |
|
There was an error when running Please check that your changes are working as intended. |
libsolutil/TestStyleViolations.cpp
Outdated
| @@ -0,0 +1,18 @@ | |||
| // SPDX-License-Identifier: GPL-3.0 | |||
| #include "libsolutil/Common.h" | |||
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.
Coding style error: Use angle brackets for includes
libsolutil/TestStyleViolations.cpp
Outdated
|
|
||
| void testFunction() | ||
| { | ||
| if(true) { |
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.
Coding style error: Missing space after keyword
libsolutil/TestStyleViolations.cpp
Outdated
|
|
||
| void testFunction() | ||
| { | ||
| if(true) { |
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.
Coding style error: Opening brace on same line as if
libsolutil/TestStyleViolations.cpp
Outdated
|
|
||
| using namespace std; | ||
|
|
||
| namespace solidity::util { |
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.
Coding style error: Missing space before opening brace in namespace
libsolutil/TestStyleViolations.cpp
Outdated
|
|
||
| namespace solidity::util { | ||
|
|
||
| const int BAD_CONST = 42; |
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.
Coding style error: const should be on the right side of the type
libsolutil/TestStyleViolations.cpp
Outdated
| { | ||
| if(true) { | ||
| int& badRef = BAD_CONST; | ||
| auto x = move(badRef); |
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.
Coding style error: Use std::move
libsolutil/TestStyleViolations.cpp
Outdated
| // SPDX-License-Identifier: GPL-3.0 | ||
| #include "libsolutil/Common.h" | ||
|
|
||
| using namespace std; |
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.
Coding style error: Do not use 'using namespace std'
Co-authored-by: Heath Dutton <heathdutton@gmail.com>
Co-authored-by: Heath Dutton <heathdutton@gmail.com>
Co-authored-by: Heath Dutton <heathdutton@gmail.com>
9e45e2b to
c0ab088
Compare
2a40b93 to
48f2db7
Compare
| # unqualified move()/forward() checks, i.e. make sure that std::move() and std::forward() are used instead of move() and forward() | ||
| preparedGrep "move\(.+\)" | grep -v "std::move" | grep -E "[^a-z]move" | ||
| preparedGrep "forward\(.+\)" | grep -v "std::forward" | grep -E "[^a-z]forward" | ||
| ) | grep -E -v -e "^[a-zA-Z\./]*:[0-9]*:\s*\/(\/|\*)" -e "^test/" || true |
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.
This regex was updated to account for the new [Error description] prefix added by addPrefix. The pattern now matches lines starting with text enclosed in square brackets before the file path.
Part of CI refactor suggested by #16136 and #16253 (comment). This PR replaces our current code-style setup by a github action. After merging we can completely remove the need of a bot account and remove the GITHUB_ACCESS_TOKEN from CircleCI.