-
Notifications
You must be signed in to change notification settings - Fork 30k
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
tools: clarify commit message linting #23742
Conversation
.travis.yml
Outdated
@@ -13,7 +13,7 @@ matrix: | |||
script: | |||
- make lint | |||
# Lint the first commit in the PR. | |||
- git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx core-validate-commit --no-validate-metadata | |||
- echo 'Linting the commit message...' && git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx -q core-validate-commit --no-validate-metadata |
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.
I think that at his point you should promote this to either a script in tools
, or a Makefile
target.
That will also allow manual checks
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.
Maybe. There is, however, a reliance on a Travis-specific environment variable....
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.
A script could be parameter based...
Or maybe move the logic into core-validate-commit
.
It would be nice to be able to just do npx core-validate-commit
and get an answer.
Could you add:
|
Clarify in Travis results that the commit message linting is for the commit message and not something else.
I'd prefer a non-obfuscated URL. |
It's kinda horible |
I put in the shortened URL for now. We can always switch in the non-obfuscated one later, but it is about 110 characters long.... 😱 |
(By the way: Thanks everyone for patiently working with this. I know it's caused more questions and confusion than anything else up until now, which is unfortunate. Hopefully this and the other thing that just landed will make it valuable...) |
Ohh I just had an idea how to workaround Travis's Pass/Fail limitation. Use the node-bot; curl it with parameters so it could post a comment in the PR thread. |
I'm +1 for fast-tracking this |
That's one fast-track approval from @refack. If another Collaborator also feels this should be fast-tracked, please 👍 this comment or otherwise indicate so. Thanks! |
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: nodejs#23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Landed in 2cb2597 |
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the commit message and not something else. PR-URL: #23742 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Clarify in Travis results that the commit message linting is for the
commit message and not something else.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes