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

feat: handle multiple commits in a single message #2358

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

dgokcin
Copy link
Contributor

@dgokcin dgokcin commented Aug 15, 2024

Summary

This PR enhances the breaking change detection mechanism and improves the parsing of commit messages to handle multiple conventional commits and nested commit blocks mentioned in #2357

Changes

  • Added additional breaking change detection from the commit body in toConventionalChangelogFormat and postProcessCommits functions.
  • Enhanced splitMessages function to:
    • Separate conventional commits within the main message.
    • Extract nested commits enclosed in BEGIN_NESTED_COMMIT/END_NESTED_COMMIT blocks.
    • Preserve the original message structure outside of nested commit blocks.
    • Handle multiple nested commits and conventional commits in a single message.
  • Updated tests in test/commits.ts to reflect the changes and added new test cases for multiple commits in a single message.
  • Added a new fixture multiple-commits-single-message.txt for testing multiple commits in a single message.

Additional Notes

  • The changes ensure that breaking changes mentioned in the commit body are correctly appended to the breaking change notes.
  • The splitMessages function now correctly handles and separates multiple conventional commits and nested commit blocks, improving the accuracy of commit parsing.
  • Updated test cases ensure the robustness of the new functionality and maintain backward compatibility.
  • First time contributing to release-please so please review carefully as the splitMessages function seems to be a critical function 🙃

Fixes #2357

@dgokcin dgokcin requested review from a team as code owners August 15, 2024 13:28
Copy link

google-cla bot commented Aug 15, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@dgokcin dgokcin changed the title Enhance Breaking Change Detection and Commit Parsing feat: handle multiple commits in a single message Aug 15, 2024
@dgokcin dgokcin force-pushed the support-multiple-commits branch from 61939a8 to 736a363 Compare August 15, 2024 13:44
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 11, 2024
Copy link
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Thanks for this and sorry for the delay in reviewing.

For the splitting commit messages, this seems ok to me -- we're splitting on a newline and then the next line looks like a conventional commit message. The scenario we're intentionally trying to avoid is GitHub's default squash/merge message where incremental commit messages might be parsed which we don't want (example: https://github.com/googleapis/release-please/blob/main/test/fixtures/commit-messages/1257-breaking-change.txt). It seems this feature still ignores the subcommit messages because the next line doesn't start with the conventional commit looking message.

For the additional breaking change detection, please split it into a separate PR and add a test for it. Additionally, the current breaking change detection is handled by the conventional-commits/parser library which uses a grammar to actually conform to the conventionalcommits.org spec. I'm not sure we want to add this additional way of marking a breaking change.

@dgokcin
Copy link
Contributor Author

dgokcin commented Sep 11, 2024

Hi @chingor13, thanks for the review.

I want to clarify the necessity of including the breaking change detection lines in src/commit.ts (Lines 172 to 184 and 329 to 344) from this branch. Without these lines, the following two tests fail, which is why I felt it was essential to add the additional checks.

  1. "DefaultChangelogNotes buildNotes with commit parsing should handle meta commits"

    • This test fails because the generated changelog is missing the "BREAKING CHANGES" section. The expected output includes a breaking change for the recaptchaenterprise component, but the actual output does not contain this section, leading to a mismatch.
  2. "parseConventionalCommits parses meta commits"

    • This test fails because it expects a commit to be recognized as a breaking change (true), but it is incorrectly identified as false. This discrepancy arises from the lack of additional breaking change detection logic, which is crucial for accurately parsing and recognizing breaking changes in commit messages.

From my understanding, the library you mentioned is not capable of detecting breaking changes in the body of the commit out of the box. If it were, I would expect the tests to pass without the additional checks. By including these lines, I ensure that the tests pass and that the changelog accurately reflects the intended breaking changes, thus maintaining the integrity of the commit parsing process and stay backward compatible.

I would be happy to try out any ideas you might have to keep the scope of this PR limited to the splitting of the commit messages, without including any breaking change detection. If you have any suggestions on how to accomplish this without affecting the breaking change logic, I would appreciate your input.

@dgokcin dgokcin force-pushed the support-multiple-commits branch 2 times, most recently from 8f21f41 to e650ec7 Compare September 11, 2024 23:34
- 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
- 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
@dgokcin dgokcin force-pushed the support-multiple-commits branch from e650ec7 to d39e3fb Compare September 17, 2024 09:10
@chingor13 chingor13 added this pull request to the merge queue Sep 17, 2024
Merged via the queue into googleapis:main with commit ec41c38 Sep 17, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: splitMessages function fails to handle multiple conventional commits
2 participants