-
-
Notifications
You must be signed in to change notification settings - Fork 4
feat: parse CHANGELOG entry: and no-changelog
#247
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
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
CHANGELOG entry: and no-changelog
|
@SocketSecurity ignore @octokit/[email protected] |
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 this PR @HowardBraham , here are a few comments:
- Generating the changelog with auto-changelog adds an empty line after each title. However, this is not accepted by “yarn lint:changelog”.
- We could ensure all changelog entries start with lowercase for consistency.
- We could introduce an "undocumented" category for commits without changelog entries (different from “uncategorized” category). Currently commits without changelog entries and commits with changelog entries are all mixed together, which gives the impression that commits without changelog entries are ok and don’t need to be reviewed, while in most cases, they’re not user-facing and need to be reviewed. --> We don't need this anymore since we've made the changelog entries mandatory (see Slack message)
- We could automatically ignore commits including the following strings:
- “Bump main version to”
- “changelog” or “Changelog”
- “merge” or “Merge”
- “cp-“
- “cherry-pick”
- “INFRA-”
- “New Crowdin translations”
- “e2e”
- “flaky test”
- “test” (I’m hesitant about this one but in the vast majority of cases, it can be ignored)
- “ci:”, “release:” (I think that’s already ignored)
- Not sure how useful that is but in our current changelog (example here), all entries start with:
- “feat: ” if they’re in the “### Added” section
- “update: ” if they’re in the “### Changed” section
- “fix: ” if they’re in the “### Fixed” section
- Maybe we should generate something similar here?
- This also means if a commit is “feat(something): feature description”, we should only add “feat: feature description” to the changelog, and get rid of “(something)”
- Finally, I noticed “metamask-extension” is hardcoded in the PR, which means it wouldn’t work for metamask-mobile repo. We should use a parameter to make it repo-agnostic.
I know about this, and it's why in the
Actually I think we should go in the opposite direction and start all of them with an UPPERCASE. This is what we used to do, if you look at
Is this going to resolve now that changelog entries are mandatory?
This makes sense.
I think this is actually an unwanted temporary relic. In
Oh whoops, you're right, I was using that for testing. |
b9bf454 to
7291552
Compare
|
7291552 to
2988fe3
Compare
|
Thanks for the changes. I tried it out, here's the output I get for It looks good, but I think we can make some things even cleaner.
|
src/update-changelog.ts
Outdated
| const keywordsToIndicateExcludedLowerCase = keywordsToIndicateExcluded.map( | ||
| (word) => word.toLowerCase(), | ||
| ); | ||
|
|
||
| return keywordsToIndicateExcludedLowerCase.some((word) => | ||
| _description.includes(word), | ||
| ); |
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.
nit: might shave off a bit of time complexity?
| const keywordsToIndicateExcludedLowerCase = keywordsToIndicateExcluded.map( | |
| (word) => word.toLowerCase(), | |
| ); | |
| return keywordsToIndicateExcludedLowerCase.some((word) => | |
| _description.includes(word), | |
| ); | |
| return keywordsToIndicateExcluded.some((word) => { | |
| const pattern = new RegExp(word, 'u'); | |
| return Boolean(_description.match(pattern)); | |
| }); |
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.
You were on to something here, but actually there was an even better time complexity solution, implemented in 3ea27a8
| // Create a regex pattern that matches any of the ConventionalCommitTypes | ||
| const typesWithPipe = conventionalCommitTypes.join('|'); | ||
| const conventionalCommitPattern = new RegExp( | ||
| `^(${typesWithPipe})\\s*(\\([^)]*\\))?:.*$`, |
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.
Nice to see that parentheses e.g. build(deps): are supported.👍
MajorLift
left a comment
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.
This should be a great qol improvement for all contributors! LGTM
|
Hi @howard.braham,
I also re-tested everything locally and it looks good to me, I guess we can merge now. |
MajorLift
left a comment
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!
| * @param repoUrl - The URL for the GitHub repository. | ||
| * @returns The repository name, or null if it could not be determined. | ||
| */ | ||
| function extractRepoName(repoUrl: string): string { |
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.
| function extractRepoName(repoUrl: string): string { | |
| function extractRepoName(repoUrl: string | undefined): string { |
Progresses: MetaMask/MetaMask-planning#5686
CHANGELOG entry:in the PR description, and looks forno-changeloglabel, then applies this to the generated CHANGELOGConventionalCommitType.RELEASENew parameters added: