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

PRChecker: Check that semver-major commits have an explanation of breaking changes #108

Closed
refack opened this issue Nov 11, 2017 · 7 comments
Labels
enhancement Things that enhances functionality, provided by node-core-utils feature request New features for node-core-utils pr-checker Issues related to pr checker stale

Comments

@refack
Copy link
Contributor

refack commented Nov 11, 2017

Refs: nodejs/node#16846

@Tiriel
Copy link
Contributor

Tiriel commented Nov 11, 2017

Funnily enough, I saw this discussion, and thought "When this lands, this will be a perfect job for the pr_checker".

Anyway: 👍

@Tiriel Tiriel added enhancement Things that enhances functionality, provided by node-core-utils feature request New features for node-core-utils labels Nov 11, 2017
@priyank-p priyank-p changed the title PRCheker: Check that semver-major commits have an explanation of breaking changes PRChecker: Check that semver-major commits have an explanation of breaking changes Nov 12, 2017
@joyeecheung
Copy link
Member

joyeecheung commented Nov 12, 2017

To me this is not the job for pr checker, but the job for core-validate-commit: checking commit messages.

@Tiriel
Copy link
Contributor

Tiriel commented Nov 12, 2017

True! I had forgotten we had other tools 😄

@refack
Copy link
Contributor Author

refack commented Nov 13, 2017

To me this is not the job for pr checker, but the job for core-validate-commit: checking commit messages.

True! I had forgotten we had other tools

It's not a trivial separation 🤔 - this might not be a simple formatting rule like what core-validate-commit check, it might need to live in the PR description (like Fixes) and require the author's to help...

As I see it core-validate-commit:

  1. Checks formatting rules - all of which the lander can fix (maybe even automatically)
  2. Is run last in the landing process - just before a git push - so it is kind of late to stop the land process and ask for new information

IMHO such a failure should block the PR from being ready to land earlier, so that the lander could ask for more information from the author.

@priyank-p
Copy link
Contributor

priyank-p commented Nov 13, 2017

@refack for now we should probably wait for Breaking: and the length requirement to land in core first or something like that in CONTRIBUTING.md and then maybe we can implement this?

@refack
Copy link
Contributor Author

refack commented Nov 13, 2017

@cPhost in term of ❌ / ✔️ I agree.
I also think that having the tool report on the rules for uncommon cases would be very helpful... let's think about this, try to find a smart way to do that.

@joyeecheung joyeecheung added the pr-checker Issues related to pr checker label Jan 18, 2018
@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Things that enhances functionality, provided by node-core-utils feature request New features for node-core-utils pr-checker Issues related to pr checker stale
Projects
None yet
Development

No branches or pull requests

5 participants