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

The changelog generator should parse multiple type comments from merge commit. #7171

Closed
jodator opened this issue Jul 23, 2019 · 10 comments · Fixed by ckeditor/ckeditor5-dev#640
Assignees
Labels
package:dev type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@jodator
Copy link
Contributor

jodator commented Jul 23, 2019

The general rule of a thumb is that a merge should only introduce one change (atomic, precise, other blah, blah) but there are times that this would be useful.

Allow us to create merge commits like this:

Feature: Introduce a Foo.foo feature. Closes #11123.
Fix: Fixes Bar braking baz. Closes #11111.

ATM only multiple BREAKING CHANGE: are parsed (if I get this correctly).

@Reinmar
Copy link
Member

Reinmar commented Jul 23, 2019

This would be useful. Intuitively guys have been defining multiple types for their PRs since we started using the changelog generator.

PS. The "get next version" release util will also need to be updated.

@pomek
Copy link
Member

pomek commented Jul 24, 2019

If you really would like to introduce this change, we need to rewrite the whole engine that collects commits. Now we're using conventional-changelog which expect that a single commit describes a single change.

As a workaround, I prefer to make an empty commit directly after the merging one with the second prefix.

git commit --allow-empty -m 'Fix: Whatever introduced in [hash: 7 or 40 characters].'

If we go with the later one, we need to replace full hash with short hash and short hash with a URL to the proper commit on GH.

Another solution that came to my mind is using a NOTE: and describe why the bug has been fixed when the feature has been added.

PS: Multiple BC and notes are parsed because single commit can break a lot of things.

@pomek
Copy link
Member

pomek commented Jul 24, 2019

I didn't mention that conventional-changelog does much more – collect commits, parsing and transforming them, and passes all to handlebars.

@jodator
Copy link
Contributor Author

jodator commented Jul 24, 2019

If you really would like to introduce this change, we need to rewrite the whole engine that collects commits. Now we're using conventional-changelog which expect that a single commit describes a single change.

That's a bummer :( But couldn't we use NOTE: and then parse this somehow? E.g.:

Feature: This feature implements foo. Closes #11123.

NOTE: Fix: Bar no longer breaks baz. Closes #11111.

@pomek
Copy link
Member

pomek commented Jul 24, 2019

We can transform a single commit as an array that contains two commits. This is not a problem because we already modifying the single commit so we can do whatever we want with this. But other parts of the tool don't understand why it will receive the array instead of an object. If it fails here, other parts won't work properly.

@jodator
Copy link
Contributor Author

jodator commented Jul 24, 2019

@pomek I didn't dig into internals - but I'm thinking about processing the NOTE: differently (later on I guess then splitting up one commit to two. The above idea is to have NOTE: parsed as BREAKING CHANGE and then when the changelog is generated parse NOTE: contents as "virtual" commit.

I wonder if this would be a quick "fix" for this issue - probably we could parse string after NOTE: using the tools directly (or create another "commit" object and push it to the process line). Again - I don't know the internals so it might be hard/time-consuming to do.

@pomek
Copy link
Member

pomek commented Jul 24, 2019

There is no option to add another commit to the result array (of commits). That's the problem. Wherever we will define additional info for the virtual commit, we have no option to pass the virtual commit through the process line.

@Reinmar
Copy link
Member

Reinmar commented Jul 24, 2019

Hm... I forgot about conventional changelog. So, I'd avoid hacking it or the way how we define commit messages (I don't like the proposal with Note: Fix:) because it should be intuitive.

Instead, if this is a limitation that we have, I'd rather see a GH hook which lints the proposed commit message. It can explain it to you how to format your commit message if you did a couple of things at once.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-dev May 18, 2020
@mlewand mlewand added this to the nice-to-have milestone May 18, 2020
@mlewand mlewand added type:improvement This issue reports a possible enhancement of an existing feature. package:dev labels May 18, 2020
@pomek pomek self-assigned this May 20, 2020
@pomek
Copy link
Member

pomek commented May 20, 2020

After merging ckeditor/ckeditor5-dev#640, multi-messages in a single commit will be supported.

mlewand added a commit to ckeditor/ckeditor5-dev that referenced this issue May 20, 2020
Feature (env): Support for multi-entries messages in the single commit and scoped changes. Closes [ckeditor/ckeditor5#7207](ckeditor/ckeditor5#7207). Closes [ckeditor/ckeditor5#7171](ckeditor/ckeditor5#7171). See https://ckeditor.com/docs/ckeditor5/latest/framework/guides/contributing/git-commit-message-convention.html.

Feature (env): Added new utils that help to collect commits, parsing them, and generating the changelog.

*   The util for generating changelog from commits (those must be specified as an argument). See `/lib/release-tools/utils/generatechangelog.js`
*   The util for collecting commits. See `/lib/release-tools/utils/getcommits.js`
*   The util for suggesting new version based on commits. See `/lib/release-tools/utils/getnewversiontype.js`

Feature (env): Task `generateChangelogForSinglePackage()` supports new options: `from` - a commit or tag for collecting commits since the last release, `highlightsPlaceholder` - whether to add "Release highlights" placeholder in the changelog, `collaborationFeatures` - whether to add a URL to collaboration features changelog.

Fix (env): The `getChangedFilesForCommit()` util filters files returned by the Git command. It won't return an empty string anymore.

Other (env): Commits in the changelog will display the word `commit` instead of the first 7 characters from the commit's hash. In big repositories (the number of commits is huge), 7 characters are not unique anymore.

Other (env): `Closes` references will be merged into a single entry. Github does not support such references (`Closes x, y`) but it can be simplified during the commit's transformation.

Other (env): The `provideVersion()` util from `lib/release-tools/utils/cli.js` allows disabling returning `skip` version by setting its option `disableSkipVersion` to `true`.

Other: Removed `lerna` and all its files from the project. Now the release process is handled by our tools. The entire repository will follow the same rules as `ckeditor5.` Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/versioning-policy.html.

MAJOR BREAKING CHANGES (env): Removed `generateChangelogForSubPackages()` task. Use `generateChangelogForMonoRepository()` instead.

MAJOR BREAKING CHANGES (env): Removed `generateChangelogForSubRepositories()` task. Use `generateChangelogForMonoRepository()` instead if your repository is a monorepository.

MINOR BREAKING CHANGES (env): The util `getPackagesPaths()` does not check whether packages are defined as `dependencies` in `package.json` in the main repository.

MINOR BREAKING CHANGES (env): Task `generateChangelogForSinglePackage()` does not accept options: `newVersion`, `disableMajorBump`, `isInternalRelease`, `indentLevel`, `useExplicitBreakingChangeGroups` anymore. The task should be used for generating the changelog for the single repository.

MAJOR BREAKING CHANGES (env): Removed `generateSummaryChangelog()` task.

MINOR BREAKING CHANGES (env): Removed `hasMajorBreakingChanges()` and `hasMinorBreakingChanges()` utils from `/lib/release-tools/utils/changelog.js` helper.

MINOR BREAKING CHANGES (env): Removed `getSubPackagesPaths()` util.

MINOR BREAKING CHANGES (env): Removed the `getNewReleaseType()` util. Use `getCommits()` and `getNewVersionType()` instead.

MINOR BREAKING CHANGES (env): Removed support for the `NOTE` type of commit's notes.

MINOR BREAKING CHANGES (env): Moved all utils from `/lib/release-tools/utils/transform-commit` to `/lib/release-tools/utils`.

MINOR BREAKING CHANGES (env): Renamed `getSubRepositoriesPaths()` util to `getPackagesPaths()`.
@mlewand mlewand modified the milestones: nice-to-have, iteration 32 May 20, 2020
@pomek
Copy link
Member

pomek commented May 20, 2020

Introduced in ckeditor/ckeditor5-dev#640.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:dev type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants