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

fix(bump): also propagate the parserOpts from args to conventionalRecommendedBump #89

Merged

Conversation

jan-mrm
Copy link

@jan-mrm jan-mrm commented Jul 27, 2023

Hi, to resolve the following behaviour (raised in the deprecated origin: conventional-changelog#690 (comment)) I've taken the idea and change of an already opened pull request conventional-changelog#821 (also from the deprecated origin) to fix it.

The bug thats observed is that when using the parserOpts and setting headerPattern to for example match the Azure DevOps prefix e.g using this regex "(?:\\(Merged PR \\d+: \\))?([a-zA-Z]+)(?:\\(([\\w$\\.\\-*\\s]*)\\))?\\!?:(.*)" features committed with e.g. "feat: abc" and then merged as "Merged PR 0: feat: abc" are classified correctly when writing them to the changelog but the bump only increases the patch. Thats the default bump. The bump uses the default config and the parserOpts seem not to be propagated.

parserOpts (and writerOpts) were added but only for the changelog lifecycle, as far as I can see: #37

Any concerns about this fix? :)

@jan-mrm
Copy link
Author

jan-mrm commented Aug 8, 2023

Just thought about test and ran the existing tests locally. Fixing the mock function, since the PR adds the third function parameter to 'conventional-recommended-bump'. I'll also have a look at the tests and what I could add for the parserOpts propagation.

@jan-mrm
Copy link
Author

jan-mrm commented Aug 9, 2023

updated the tests / mocking.
No idea for a specific test since the bump is always mocked.

@jan-mrm
Copy link
Author

jan-mrm commented Aug 22, 2023

@TimothyJones do you have some time by any chance to look at this PR at some point? :)

@TimothyJones
Copy link
Member

Apologies, I was away when this came in. I’ll take a look at this in the next 24 hours

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

Codecov Report

Merging #89 (5380470) into master (9ed2207) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master      #89   +/-   ##
=======================================
  Coverage   97.57%   97.57%           
=======================================
  Files          31       31           
  Lines        1280     1280           
=======================================
  Hits         1249     1249           
  Misses         31       31           
Files Changed Coverage Δ
lib/lifecycles/bump.js 98.61% <ø> (ø)
test/config-files.spec.js 96.87% <100.00%> (ø)
test/core.spec.js 98.31% <100.00%> (ø)
test/git.spec.js 99.17% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@TimothyJones TimothyJones merged commit bc685be into absolute-version:master Aug 22, 2023
6 checks passed
@jan-mrm
Copy link
Author

jan-mrm commented Aug 22, 2023

no worries (was just checking if it might have been overlooked) :) thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants