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

chore: add breaking change pr check ci #9050

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yeonjuan
Copy link
Contributor

@yeonjuan yeonjuan commented May 6, 2024

PR Checklist

Overview

Add CI that checks breaking change pr title and body.

  • Disallows the use of exclamation mark(!) in PR title.
  • Enforces to write “BREAKING CHANGE:” at the beginning of the PR body if the pr has breaking change label.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @yeonjuan!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented May 6, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit cde7ec4
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/663f5339678e1e0008618922
😎 Deploy Preview https://deploy-preview-9050--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 97 (🟢 up 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@yeonjuan yeonjuan marked this pull request as ready for review May 6, 2024 14:47
Comment on lines 30 to 34
function checkTitle(title) {
if (/!/.test(title)) {
raiseError(`Do not use exclamation mark ('!') in your PR Title.`);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, isn't this too restrictive? I can imagine the several cases when ! might be used not in the "breaking changes" position (before the semicolon)

Here are several real world PRs that have an exclamation mark in their titles:

I guess we should restrict only the use of feat(...)!: ... and feat!: .... What do you think?

.github/actions/breaking-pr-check/index.js Outdated Show resolved Hide resolved
@auvred auvred added the awaiting response Issues waiting for a reply from the OP or another party label May 6, 2024
@yeonjuan yeonjuan requested a review from auvred May 9, 2024 14:57
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label May 9, 2024
Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

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

One small note on PR title RegExp, otherwise LGTM!

}

function checkTitle(title) {
if (/^[a-z]+(\([a-z]+\))?!: /.test(title)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (/^[a-z]+(\([a-z]+\))?!: /.test(title)) {
if (/^[a-z]+(\([a-z-]+\))?!: /.test(title)) {

Without the dash, this regexp won't catch feat(eslint-plugin)!: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I fixed it cde7ec4

@auvred auvred added the awaiting response Issues waiting for a reply from the OP or another party label May 11, 2024
Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

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

A man has his hands on his hips and he points at us, smiling and saying, "That's good. That's damn good", as his smile grows larger.

@auvred auvred added 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready and removed awaiting response Issues waiting for a reply from the OP or another party labels May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready
Projects
None yet
2 participants