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

Allow "Unreleased" commit group to be renamed #117

Merged
merged 2 commits into from
Sep 19, 2018
Merged

Allow "Unreleased" commit group to be renamed #117

merged 2 commits into from
Sep 19, 2018

Conversation

alex-pex
Copy link
Contributor

@alex-pex alex-pex commented Jul 10, 2018

Allow "Unreleased" commit group to be renamed

  • It can be set with a value, like --next-version="Incoming in the next release"
  • It can be infered on package.json/lerna.json, like --next-version-from-metadata (it is based on version field, which is useful when lerna-changelog is used on npm version hook)

Tests updated, Lint OK

@Turbo87
Copy link
Contributor

Turbo87 commented Jul 10, 2018

@alex-pex wouldn't it be easier to just modify the output of lerna-changelog or are you using the output in an automated way?

@alex-pex
Copy link
Contributor Author

alex-pex commented Jul 10, 2018

Indeed, I could modify the output using a custom script around lerna-changelog, but I wanted something more integrated. https://github.com/conventional-changelog/conventional-changelog already uses version value when generating the changelog, which is convenient as a publish hook.

lerna-changelog is just a step in my publishing toolchain.

src/cli.ts Show resolved Hide resolved
src/cli.ts Outdated
.example(
'lerna-changelog --next-version="Next: 0.4.0"',
'create a changelog for the changes after the latest available tag, under "Next: 0.4.0" section'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need a dedicated example for this option. I'd prefer it if we kept it as it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok 👍

src/cli.ts Outdated
@@ -45,6 +56,7 @@ export async function run() {
let options = {
tagFrom: argv["from"] || argv["tag-from"],
tagTo: argv["to"] || argv["tag-to"],
nextVersion: argv["next-version"] === "" ? true : argv["next-version"],
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to change the default behavior to "autodiscover", which would be a breaking change. the default (at least for now) should keep using "Unreleased".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • when the parameter is not set, the value is undefined (default, the text will be "Unreleased")
  • when the parameter is set without value, it equals empty string, which is typecasted as true (the lib will find the version by itself)
  • when the parameter is set WITH a value, it uses it as the next version label

That's why I'm checking argv["next-version"] === "" instead of !argv["next-version"]

@Turbo87
Copy link
Contributor

Turbo87 commented Jul 11, 2018

I'd prefer it if we'd only use dedicated CLI options for this, but not read it from the nextVersion option in the config.

I'd imagine something like this:

lerna-changelog  # uses "Unreleased"
lerna-changelog --next-release "v0.3.0"  # uses "v0.3.0"
lerna-changelog --next-release-from-metadata  # uses whatever is in `package.json`

@alex-pex
Copy link
Contributor Author

What whould prevent the user from doing

lerna-changelog --next-release "v0.3.0" --next-release-from-metadata

That's why I used a single all-purpose parameter. Maybe a special value like --next-release="auto" would work ?

@Turbo87
Copy link
Contributor

Turbo87 commented Jul 22, 2018

What whould prevent the user from doing

shouldn't be hard to show a warning for that case

That's why I used a single all-purpose parameter. Maybe a special value like --next-release="auto" would work ?

I'm not a big fan of such magic constants. I'd prefer to have explicit parameters for the different modes.

@alex-pex
Copy link
Contributor Author

I'll try to work on the requested changes as soon as possible

@alex-pex
Copy link
Contributor Author

alex-pex commented Aug 3, 2018

I've fixed the issues reported

Copy link
Contributor

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

looking good, thanks and sorry for the long review time :)

@Turbo87 Turbo87 merged commit 6fc2941 into lerna:master Sep 19, 2018
@alex-pex alex-pex deleted the feature/set-next-version branch September 20, 2018 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants