-
Notifications
You must be signed in to change notification settings - Fork 40
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
Only bump and create changelog if there ir a breaking change, feature or fix #99
base: master
Are you sure you want to change the base?
Conversation
If someone it's willing to try this without using my forked repo here its a commit-and-tag-version+11.2.3.patch
|
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.
Thank you so much! This is an often requested feature
It would be great to add tests, and I think the behaviour should be behind a config flag (otherwise it’s a breaking change).
Cool, I could add tests and make it a config flag. Maybe |
Sorry for the slow reply, I think |
Looks like some lint failed too. Should be easily fixed with a
Definitely. |
Perfect |
@TimothyJones added added config flag and linted files. I tried to add tests, but I'm struggling to make the test go through the |
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.
I don't think the commit messages are mocked, instead the decision that conventional-recommended-bump makes is mocked.
This means if you're relying on conventional-recommended-bump calling this code, then the mock might need to be updated. At that point we're kind of marking our own exam. It might be better not to mock conventional-recommended-bump, and instead mock the modules that provide the commits (git-raw-commits
and git-semver-tags
). I'm not wild about that, as it means the tests need to know a lot about conventional-changelog, but most of this code already does, so it's probably ok.
The easiest tests to read are in core.spec.js
-the mock for conventional-changelog is set up in the mock
function in that test. You can compare with say git.spec.js
, which mocks slightly different parts of the dependencies.
I think it needs a test for:
- When there are no changes and the option is not set (should bump)
- When there are changes and the option is not set (should bump)
- When there are no changes and the option is set (should not bump)
- When there are changes and the option is set (should bump)
// eslint-disable-next-line no-async-promise-executor | ||
return new Promise(async (resolve, reject) => { |
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.
// eslint-disable-next-line no-async-promise-executor | |
return new Promise(async (resolve, reject) => { | |
return new Promise((resolve, reject) => { |
Unless I'm missing something, you don't need this async
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.
Yes I'll remove it it's from an old implementation.
...args.noBumpWhenEmptyChanges && { | ||
whatBump (commits) { | ||
return _whatBump(presetOptions, commits) | ||
} |
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.
...args.noBumpWhenEmptyChanges && { | |
whatBump (commits) { | |
return _whatBump(presetOptions, commits) | |
} | |
...(args.noBumpWhenEmptyChanges | |
? { | |
whatBump (commits) { | |
return _whatBump(presetOptions, commits) | |
} | |
} | |
: {}) |
^ I think this is a bit more idiomatic
let configsToUpdate = {} | ||
|
||
function _whatBump (config, commits) { |
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.
Should this perhaps be named noBumpWhenEmptyChanges
?
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #99 +/- ##
==========================================
- Coverage 97.57% 96.09% -1.49%
==========================================
Files 31 31
Lines 1280 1305 +25
==========================================
+ Hits 1249 1254 +5
- Misses 31 51 +20
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
What is the status of this PR? Will it land? |
Hi, are there any updates on this? |
As above, I think it needs a little more work - if no one has done it by the next time I have a block of free time, I’ll look in to it. |
This PR kind of solves #43 because removes completely the possibility from the package the ability of bumping file version and updating changelog if there are no commits that follow conventional commits.
Let me know if there is something I could add to explain what's the code doing. But what mainly is happening it's that I copied the
whatBump
function from the preset ofconventional-changelog-preset-loader
and tweaked to return an empty release if there are no breaking, feature or fixes in the commits since last release.This repo would greatly benefit from an updated ecma code version or even typescript, and updating dependencies.