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

PR hygiene checking #910

Merged
merged 28 commits into from
Feb 3, 2021
Merged

PR hygiene checking #910

merged 28 commits into from
Feb 3, 2021

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented Jan 4, 2021

fixes #782

Here's what it looks like if the PR title was Update index.html
Screen Shot 2021-01-04 at 10 37 24 AM

And if all is well:
Screen Shot 2021-01-04 at 10 37 56 AM

Not rocket science but hopefully a start.

@peterbe peterbe changed the title pr lint check Update index.html Jan 4, 2021
@peterbe peterbe changed the title Update index.html PR hygiene checking Jan 4, 2021
@peterbe peterbe marked this pull request as ready for review January 4, 2021 15:38
@peterbe peterbe requested review from a team and fiji-flo January 4, 2021 15:38
@nschonni
Copy link
Contributor

nschonni commented Jan 5, 2021

@peterbe
Copy link
Contributor Author

peterbe commented Jan 5, 2021

https://github.com/marketplace/actions/conventional-pr-title-alt might be another interesting option

Edit: or one of these https://github.com/marketplace?type=actions&query=title

I looked at most of those and didn't feel attracted to any of them. They all seemed overly complicated or just not perfect. And besides, I quite like the idea of building it "from scratch" because then you're limitless in what you want to implement. Our current logic is a bit odd in that it's so oddly specific. And we can now quite easily express our business logic with simple string-checking if-statements in JavaScript rather than custom regexes in a Yaml file.

Copy link
Contributor

@escattone escattone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good start to me, thanks @peterbe!

pr-lint/src/index.js Outdated Show resolved Hide resolved
@peterbe
Copy link
Contributor Author

peterbe commented Jan 6, 2021

ping @fiji-flo

@peterbe
Copy link
Contributor Author

peterbe commented Jan 12, 2021

One thing I've found for myself is that when I do a lot of flaw fixing PRs, I never type in a description. Because it's rarely useful when the summary is sufficient.

@Ryuno-Ki
Copy link
Collaborator

This looks ready to merge (from a CI point of view)

@peterbe
Copy link
Contributor Author

peterbe commented Jan 27, 2021

I changed my mind. I made it less controversial so that it doesn't require that the description is non-empty.
I've made many PRs on the content and I often find that once you've typed a decent title, there's really nothing else worth saying in the description. At least not worthy enough for it to block in CI.

@peterbe
Copy link
Contributor Author

peterbe commented Jan 27, 2021

@fiji-flo Do you mind if I merge without your review?

@escattone escattone merged commit bcb104c into mdn:main Feb 3, 2021
@peterbe peterbe deleted the 782-pr-lint-check branch February 3, 2021 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update index.html title not very helpful
4 participants