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

Bug: splitMessages function fails to handle multiple conventional commits #2357

Closed
dgokcin opened this issue Aug 14, 2024 · 0 comments · Fixed by #2358
Closed

Bug: splitMessages function fails to handle multiple conventional commits #2357

dgokcin opened this issue Aug 14, 2024 · 0 comments · Fixed by #2358
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@dgokcin
Copy link
Contributor

dgokcin commented Aug 14, 2024

Thank you to the creators of release-please for your hard work and dedication to the project.

I wanted to bring to your attention an issue with the current implementation of the parsing logic in src/commit.ts. It appears that the logic does not handle multiple conventional commits within a single squash merge correctly. The implementation seems to rely heavily on specific formatting and structure that does not fully align with the Conventional Commits specification.

For example, problems arise when commit messages include bullet points, empty lines, or varying indentations, as demonstrated in the experiment examples provided. Additionally, the documentation does not mention the requirement for body messages to be indented by two spaces, as seen in some examples in the repository's README. This discrepancy suggests that the current logic may not fully adhere to the specification, which the repository aims to support.

Here is the conventional commit format which I believe the examples below all fit in.

<type>[optional scope]: <description>

[optional body]

[optional footer(s)]

Here are some examples from the experiments I made

Slightly modified example of the example in the README - Parsed correctly, however there needs to be a new line between body and title
feat: adds v4 UUID to crypto

This adds support for v4 UUIDs to the library.

fix(utils): unicode no longer throws exception
- some more stuff

feat(utils): update encode to support unicode
- does stuff
- more stuff
Same example except this time with bullets are at the body (Parsed Incorrectly)
feat: adds v4 UUID to crypto

This adds support for v4 UUIDs to the library.

fix(utils): unicode no longer throws exception

- some more stuff

feat(utils): update encode to support unicode

- does stuff
- more stuff
Same example with an empty line between the description and the body (Parsed Incorrectly - only the first commit is treated as a conventional commit)
feat: adds v4 UUID to crypto

This adds support for v4 UUIDs to the library.

fix(utils): unicode no longer throws exception

- some more stuff

feat(utils): update encode to support unicode

- does stuff
- more stuff
Multiple commits with a simple body (Parsed Incorrectly - only the first commit is treated as a conventional commit)
feat: adds v4 UUID to crypto

This adds support for v4 UUIDs to the library.

fix(utils): unicode no longer throws exception

some more stuff

feat(utils): update encode to support unicode

does stuff
more stuff

Additional Details

  • I found out that the BEGIN_NESTED_COMMIT and END_NESTED_COMMIT could be used to customize the nested commits, however I think it would be awesome if release please can handle these automatically.
  • I feel like the splitMessages function could be modified to support the aforementioned commits and improve the parsing. A simple change like below supports the commit types I mentioned above.(It brakes only 5 tests)
function splitMessages(message: string): string[] {
  const parts = message.split('BEGIN_NESTED_COMMIT');
  const messages = [parts.shift()!];
  for (const part of parts) {
    const [newMessage, ...rest] = part.split('END_NESTED_COMMIT');
    messages.push(newMessage);
    messages[0] = messages[0] + rest.join('END_NESTED_COMMIT');
  }

  // Split the first message into conventional commits
  const conventionalCommits = messages[0].split(/\n(?=(?:feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.*?\))?: )/);

  return [...conventionalCommits, ...messages.slice(1)];
}

Environment details

  • OS: macos sonoma 14.5
  • Node.js version: v22.2.0
  • npm version: 10.7.0
  • release-please version: 16.12.0

Steps to reproduce

  1. Create a new txt file from the examples above in the test/fixtures/commit-messages/ directory
  2. Create a new test in test/commits.ts with the following content
  it('test custom commits', async () => {
  const commits = [buildCommitFromFixture('name-of-the-fixture')];
  const conventionalCommits = parseConventionalCommits(commits);
  console.log('Conventional Commits:');
  console.log(JSON.stringify(conventionalCommits, null, 2));
  expect(conventionalCommits).lengthOf(1);
  expect(conventionalCommits[0].breaking).to.be.true;
  expect(conventionalCommits[0].message).not.include('I should be removed');
});

3. Run `npm test` > out.txt
5. Search the `out.txt` file for "Conventional Commits:" to inspect the conventionalCommits array.
@dgokcin dgokcin added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Aug 14, 2024
dgokcin pushed a commit to dgokcin/release-please that referenced this issue Aug 15, 2024
- updated `splitMessages` function to handle multiple conventional commits within the main message
- added logic to separate conventional commits (feat, fix, docs, etc.) within the main message
- preserved the original message structure outside of nested commit blocks
- enhanced breaking change detection from commit body in `toConventionalChangelogFormat` and `postProcessCommits` functions

Resolves googleapis#2357
dgokcin added a commit to dgokcin/release-please that referenced this issue Aug 15, 2024
- updated `splitMessages` function to handle multiple conventional commits within the main message
- added logic to separate conventional commits (feat, fix, docs, etc.) within the main message
- preserved the original message structure outside of nested commit blocks
- enhanced breaking change detection from commit body in `toConventionalChangelogFormat` and `postProcessCommits` functions

Resolves googleapis#2357
dgokcin added a commit to dgokcin/release-please that referenced this issue Sep 11, 2024
- updated `splitMessages` function to handle multiple conventional commits within the main message
- added logic to separate conventional commits (feat, fix, docs, etc.) within the main message
- preserved the original message structure outside of nested commit blocks
- enhanced breaking change detection from commit body in `toConventionalChangelogFormat` and `postProcessCommits` functions

Resolves googleapis#2357
dgokcin added a commit to dgokcin/release-please that referenced this issue Sep 17, 2024
- updated `splitMessages` function to handle multiple conventional commits within the main message
- added logic to separate conventional commits (feat, fix, docs, etc.) within the main message
- preserved the original message structure outside of nested commit blocks
- enhanced breaking change detection from commit body in `toConventionalChangelogFormat` and `postProcessCommits` functions

Resolves googleapis#2357
github-merge-queue bot pushed a commit that referenced this issue Sep 17, 2024
* feat: handle multiple commits in a single message

- updated `splitMessages` function to handle multiple conventional commits within the main message
- added logic to separate conventional commits (feat, fix, docs, etc.) within the main message
- preserved the original message structure outside of nested commit blocks
- enhanced breaking change detection from commit body in `toConventionalChangelogFormat` and `postProcessCommits` functions

Resolves #2357

* test: new test case for parsing multiple commits

- added new test cases for parsing multiple commits from a single message
- added fixture file for multiple commits in a single message
- changed the order of the expected commits in `multiple-messages` and `1257-breaking-change` fixtures since the order of the commits in the is not reversed anymore

* test: remove duplicate test for parsing multiple commits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant