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

Only bump and create changelog if there ir a breaking change, feature or fix #99

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hazzo
Copy link

@hazzo hazzo commented Sep 12, 2023

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 of conventional-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.

@hazzo
Copy link
Author

hazzo commented Sep 12, 2023

If someone it's willing to try this without using my forked repo here its a .patch file to apply with patch-package

commit-and-tag-version+11.2.3.patch
diff --git a/node_modules/commit-and-tag-version/index.js b/node_modules/commit-and-tag-version/index.js
index 8941e65..5286607 100755
--- a/node_modules/commit-and-tag-version/index.js
+++ b/node_modules/commit-and-tag-version/index.js
@@ -82,6 +82,7 @@ module.exports = async function standardVersion (argv) {
     }
 
     const newVersion = await bump(args, version)
+    if(!newVersion) return;
     await changelog(args, newVersion)
     await commit(args, newVersion)
     await tag(newVersion, pkg ? pkg.private : false, args)
diff --git a/node_modules/commit-and-tag-version/lib/lifecycles/bump.js b/node_modules/commit-and-tag-version/lib/lifecycles/bump.js
index da72199..3122da3 100644
--- a/node_modules/commit-and-tag-version/lib/lifecycles/bump.js
+++ b/node_modules/commit-and-tag-version/lib/lifecycles/bump.js
@@ -12,19 +12,64 @@ const runLifecycleScript = require('../run-lifecycle-script')
 const semver = require('semver')
 const writeFile = require('../write-file')
 const { resolveUpdaterObjectFromArgument } = require('../updaters')
+const addBangNotes = require('conventional-changelog-conventionalcommits/add-bang-notes')
 let configsToUpdate = {}
 
+function _whatBump(config, commits) {
+  let level = 2
+  let breakings = 0
+  let features = 0
+  let fix = 0
+
+  commits.forEach(commit => {
+    addBangNotes(commit)
+    if (commit.notes.length > 0) {
+      breakings += commit.notes.length
+      level = 0
+    } else if (commit.type === 'feat' || commit.type === 'feature') {
+      features += 1
+      if (level === 2) {
+        level = 1
+      }
+    }
+    if (commit.type === 'fix') {
+      fix += 1
+    }
+  })
+
+  if (config.preMajor && level < 2) {
+    level++
+  }
+
+  if(!breakings && !features && !fix) return {}
+
+  return {
+    level,
+    reason: breakings === 1
+      ? `There is ${breakings} BREAKING CHANGE and ${features} features`
+      : `There are ${breakings} BREAKING CHANGES and ${features} features`
+  }
+}
+
 async function Bump (args, version) {
   // reset the cache of updated config files each
   // time we perform the version bump step.
   configsToUpdate = {}
-
   if (args.skip.bump) return version
   let newVersion = version
   await runLifecycleScript(args, 'prerelease')
   const stdout = await runLifecycleScript(args, 'prebump')
   if (stdout && stdout.trim().length) args.releaseAs = stdout.trim()
   const release = await bumpVersion(args.releaseAs, version, args)
+  console.log(release)
+  if(!Object.keys(release).length) {
+    checkpoint(
+      args,
+      'no commits found, so not bumping version',
+      []
+    )
+    return null;
+  }
   if (!args.firstRelease) {
     const releaseType = getReleaseType(args.prerelease, release.releaseType, version)
     const releaseTypeAsVersion = releaseType === 'pre' + release.releaseType ? semver.valid(release.releaseType + '-' + args.prerelease + '.0') : semver.valid(releaseType)
@@ -108,7 +153,7 @@ function getTypePriority (type) {
 }
 
 function bumpVersion (releaseAs, currentVersion, args) {
-  return new Promise((resolve, reject) => {
+  return new Promise(async (resolve, reject) => {
     if (releaseAs) {
       return resolve({
         releaseType: releaseAs
@@ -123,7 +168,8 @@ function bumpVersion (releaseAs, currentVersion, args) {
         preset: presetOptions,
         path: args.path,
         tagPrefix: args.tagPrefix,
-        lernaPackage: args.lernaPackage
+        lernaPackage: args.lernaPackage,
+        whatBump(commits) { return _whatBump(presetOptions, commits) }
       }, args.parserOpts, function (err, release) {
         if (err) return reject(err)
         else return resolve(release)

Copy link
Member

@TimothyJones TimothyJones left a 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).

lib/lifecycles/bump.js Show resolved Hide resolved
@hazzo
Copy link
Author

hazzo commented Sep 13, 2023

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 bumpIfChanges and make it a boolean?

@TimothyJones
Copy link
Member

Sorry for the slow reply, I think bumpIfChanges is a bit ambiguous - what about bumpOnlyWhenChangelog or noBumpWhenEmptyChanges or something, both defaulting to false?

@TimothyJones
Copy link
Member

Looks like some lint failed too. Should be easily fixed with a --fix, sorry I don't have this automation set up.

This repo would greatly benefit from an updated ecma code version or even typescript, and updating dependencies.

Definitely.

@hazzo
Copy link
Author

hazzo commented Sep 25, 2023

Sorry for the slow reply, I think bumpIfChanges is a bit ambiguous - what about bumpOnlyWhenChangelog or noBumpWhenEmptyChanges or something, both defaulting to false?

Perfect noBumpWhenEmptyChanges works for me. I'll work on this PR this weekend (add tests and lint fixes also).

@hazzo
Copy link
Author

hazzo commented Oct 1, 2023

@TimothyJones added added config flag and linted files. I tried to add tests, but I'm struggling to make the test go through the whatBump branch. My thought is that maybe conventionalRecommendedBump it's being mocked and that's why it does not go through my custom whatBump function. Could you point me into the right direction of how should I test this feature? I also don't get how git history it's mocked to get commits.

Copy link
Member

@TimothyJones TimothyJones left a 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)

Comment on lines +156 to +157
// eslint-disable-next-line no-async-promise-executor
return new Promise(async (resolve, reject) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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

Copy link
Author

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.

Comment on lines +173 to +176
...args.noBumpWhenEmptyChanges && {
whatBump (commits) {
return _whatBump(presetOptions, commits)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
...args.noBumpWhenEmptyChanges && {
whatBump (commits) {
return _whatBump(presetOptions, commits)
}
...(args.noBumpWhenEmptyChanges
? {
whatBump (commits) {
return _whatBump(presetOptions, commits)
}
}
: {})

^ I think this is a bit more idiomatic

lib/lifecycles/bump.js Show resolved Hide resolved
let configsToUpdate = {}

function _whatBump (config, commits) {
Copy link
Member

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-commenter
Copy link

codecov-commenter commented Oct 8, 2023

Codecov Report

Merging #99 (e936987) into master (1912775) will decrease coverage by 1.49%.
Report is 5 commits behind head on master.
The diff coverage is 23.07%.

❗ 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     
Files Coverage Δ
command.js 84.61% <ø> (ø)
index.js 100.00% <100.00%> (ø)
lib/lifecycles/bump.js 78.12% <20.00%> (-20.49%) ⬇️

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

@cbou
Copy link

cbou commented Apr 19, 2024

What is the status of this PR? Will it land?

@Rankarusu
Copy link

Hi, are there any updates on this?

@TimothyJones
Copy link
Member

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.

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.

5 participants