-
Notifications
You must be signed in to change notification settings - Fork 26
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
feat: add semantic PR check to js-libp2p repos #93
feat: add semantic PR check to js-libp2p repos #93
Conversation
Before merge, verify that all the following plans are correct. They will be applied as-is after the merge. Terraform planslibp2p
|
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.
Thanks for creating the PR 😄 Once we move the file, it'll be good to go from the GitHub Management point of view. I'll let others review before the merge though.
I've not used this plugin before, does it pull the list of allowed scopes from the project config or does it need configuring in the action yaml? |
|
@mpetrunic is all that's remaining for this a review? @thomaseizinger added pretty much the same thing in rust-libp2p so I think that's proof that this works. I approved but feel free to wait for @achingbrain We'd want to enable this in go-libp2p too per: libp2p/go-libp2p#1931 cc @MarcoPolo |
Yup, think we are all waiting for approves here^^ |
Co-authored-by: Laurent Senta <[email protected]>
It works quite well for us! Something I've also added is a lint for the length of the PR title to not exceed 72 chars. This might be interesting to include here as well. |
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.
cc @mxinden We should probably switch to this being managed through here as well, assuming we can agree on the set of scopes :)
feat | ||
fix | ||
chore | ||
docs | ||
deps | ||
test |
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.
We also allow refactor
and ci
. Something to consider :)
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 added those two here.
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.
LGTM
@galargh I think you've already reviewed but I don't want to merge this without your approval. PTAL |
Summary
Resolves #81
Why do you need this?
What else do we need to know?
DRI: myself
Reviewer's Checklist