-
Notifications
You must be signed in to change notification settings - Fork 97
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
New CLI options, custom tags range and tests. #31
Conversation
This PR mainly introduces a way of querying commits for a custom tags range (at the moment only commits after the last available tag are taken into consideration). Now you can specify a custom range by passing --tagFrom and/or --tagTo options (--tagTo should be a tag after --tagFrom). Additionally, as also mentioned in lerna#25, if multiple tags are present within the custom commits range, they will be grouped together a changelog entry for each tag will be created. Basic tests are also present now. Closes lerna#17,lerna#19,lerna#25
@@ -7,9 +7,10 @@ | |||
"lerna-changelog": "cli.js" | |||
}, | |||
"scripts": { | |||
"build": "babel src -d lib", | |||
"build": "npm run clean && babel src --out-dir lib --ignore src/__mocks__/GithubAPI.js,src/__mocks__/Changelog.js", |
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'm not sure if there is a better way of doing this...
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.
We can just ignore in npmignore/files? (I normally but all tests in a test folder not like jest so not sure)
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.
The problem is if I keep those mocked folders is that they will be present in /lib
as well as /src
and when jest
runs it outputs some logs. Unfortunately I couldn't find a way of configuring this with jest
, if you know of a better solution I'm all ears.
jest-haste-map: duplicate manual mock found:
Module name: GithubAPI
Duplicate Mock path: /Users/emmenko/dev/src/lerna-changelog/src/__mocks__/GithubAPI.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/Users/emmenko/dev/src/lerna-changelog/src/__mocks__/GithubAPI.js
Please delete one of the following two files:
/Users/emmenko/dev/src/lerna-changelog/lib/__mocks__/GithubAPI.js
/Users/emmenko/dev/src/lerna-changelog/src/__mocks__/GithubAPI.js
jest-haste-map: duplicate manual mock found:
Module name: Changelog
Duplicate Mock path: /Users/emmenko/dev/src/lerna-changelog/src/__mocks__/Changelog.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/Users/emmenko/dev/src/lerna-changelog/src/__mocks__/Changelog.js
Please delete one of the following two files:
/Users/emmenko/dev/src/lerna-changelog/lib/__mocks__/Changelog.js
/Users/emmenko/dev/src/lerna-changelog/src/__mocks__/Changelog.js
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.
now I wonder how our jest + babel projects are doing it then? @cpojer if have ideas
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.
We don't publish Jest's mocks to npm. They are internal to Jest, I suggest you do the same.
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 also found this thread from you @cpojer where you mention
Manual mocking is really messed up and doesn't work well.
Do you still recommend to use them or to "wait until they're fixed"?
Besides the publishing part, what about compiled sources from babel? Should we manually ignore those or is there a better way of doing it?
Thanks for the help!
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 recommend using them within one project but I do not recommend publishing them to npm.
import ConfigurationError from "./ConfigurationError"; | ||
|
||
const UNRELEASED_TAG = "___unreleased___"; |
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.
This is used for grouping unreleased commits
|
||
// CLI options | ||
this.tagFrom = options.tagFrom; | ||
this.tagTo = options.tagTo; |
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.
Those are the new CLI options
|
||
// Get all info about commits in a certain tags range | ||
const commitsInfo = this.getCommitsInfo(); | ||
const commitsByTag = this.getCommitsByTag(commitsInfo); |
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.
This is the new function to group commits by tag
"))"; | ||
}); | ||
}); | ||
}); |
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.
Nothing changed much here, just formatting
} | ||
return acc; | ||
}, {}) | ||
); |
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.
Same as before, just extracted into a separate function
const commits = execSync( | ||
// Prints "<short-hash>;<ref-name>;<summary>;<date>" | ||
// This format is used in `getCommitsInfo` for easily analize the commit. | ||
'git log --oneline --pretty="%h;%D;%s;%cd" --date=short ' + tagsRange |
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.
This is the most important part. Besides passing a custom range, the output is formatted with a specific pattern. This is necessary in order to analyse the commit.
<short-hash>;<ref-name>;<summary>;<date>
c => c === login || login.indexOf(c) > -1 | ||
) | ||
); | ||
if (login && shouldKeepCommiter && !committers[login]) { |
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.
This part includes the other option ignoreCommitters
.
tag = _refs[1].replace(/tag:\s(.*?)$/, "$1").trim(); | ||
} | ||
const message = parts[2]; | ||
const date = parts[3]; |
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.
This part has been refactored to parse the different commit info, especially the tag name
@@ -35,6 +35,8 @@ | |||
"indent": ["error", 2] | |||
}, | |||
"env": { | |||
"es6": true, | |||
"jest": true, |
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.
for the eslint config we should just use https://github.com/babel/eslint-config-babel. I would do this in a new pr after this is merged so its easier to review
awesome stuff, there's a lot here so may take a while to review |
cc @fson if curious/helpful? |
Yeah take your time, I also left some TODOs in the PR description for extra stuff that can be addressed as well. I'm open to suggestions and improvements anyway on the take of this PR, so feel free to give all the feedback you have before we can proceed further ;) |
@@ -71,6 +71,20 @@ You'll need a GitHub API [personal access token](https://github.com/settings/tok | |||
- `repo`: Your "org/repo" on GitHub | |||
- `cacheDir` [optional]: A place to stash GitHub API responses to avoid throttling | |||
- `labels`: GitHub issue/PR labels mapped to changelog section headers | |||
- `ignoreCommitters` [optional]: list of commiters to ignore (exact or partial match) |
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.
Ah yeah I guess this could be useful for bots like greenkeeper?
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.
Yep, exactly ;) We could also mention it in the CLI help
|
||
|
||
Options: | ||
--tagFrom <tag> define a custom tag to determine the lower bound of the range of commits (default: last available git tag) |
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.
Not a big deal but is --tag-from
or --tagFrom
more common for cli?
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.
In my experience, --kebab-case
(dashed) is more common for long flags. Most arg parsers will map them to the camelCase version automatically.
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.
Good point, I'll change that 👍
@@ -1,13 +1,26 @@ | |||
#!/usr/bin/env node | |||
|
|||
var chalk = require("chalk"); | |||
var lib = require("."); | |||
var argv = require("yargs").argv; |
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.
We don't have to but looks like we use meow in lerna already https://github.com/lerna/lerna/blob/master/bin/lerna.js#L6
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 actually wanted to refactor lerna's (well, at the time, asini's) usage of meow into yargs, it's got so many neat bells and whistles (with better help out of the box).
"clean": "rimraf lib", | ||
"test": "eslint index.js cli.js src/", | ||
"lint": "eslint index.js cli.js src/", |
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 you need the /
for src
response.commitSHA = sha; | ||
response.mergeMessage = message; | ||
return response; | ||
Object.assign(commitInfo, response); |
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.
This reminds me that if we have have tests we should setup travis with node 0.10 since we haven't dropped it yet. Then we can drop < node 4 in a major.
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.
The travis build is one of the TODOs yes. Which versions do you want to test? I guess:
0.10
0.12
4
6
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.
Yeah that sounds good.
I figured we can do a release with this and then after do a major bump to remove it
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.
Btw, do you want to automate the release process (e.g. semantic-release) or still keep it manual (npm publish
)? At least I would consider having a script to bump the version, tag it and publish it.
But I'm not sure how you guys are used to work and prefer to do this sorts of stuff, so yeah just asking... :)
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 haven't used semantic release and most projects like Babel I would want to do a batch of commits rather than a new release per PR. But I guess for something smaller we could like this. I don't know?
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.
No worries, I don't have much of a preference either. I would probably keep it consistent with what lerna
is using.
Anyway, not really in the scope of this PR :)
#### :rocket: New Feature | ||
* \`pkg-1\`, \`pkg-2\` | ||
* [#1](https://github.com/lerna/lerna-changelog/pull/1) This is the commit title for the issue (#1). ([@test-user](https://github.com/test-user)) | ||
* [#1](https://github.com/lerna/lerna-changelog/pull/1) This is the commit title for the issue (#1). ([@test-user](https://github.com/test-user)) |
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.
why does this have 2 of the same commit?
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.
Yeah, actually it's correct to have 2 commit entries here. The problem is that the mock returns the same value, thus you see the same message. But I want to refactor a bit more the way we're mocking stuff, making it easier to setup the tests.
I'll push some changes soon.
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.
Cool! Would be awesome to have a test for 2 tags (New feature/bugfix or something) and see the formatting
basically just the places where the formatting might be different
|
||
#### Committers: 0 | ||
" | ||
`; |
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.
If the tag is empty should it not print (same if no committers?)
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.
Good point, I'll look into it 👍
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.
Done f321146
|
||
var Changelog = lib.Changelog; | ||
var ConfigurationError = lib.ConfigurationError; | ||
if (argv.help) { |
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.
If you are going to use yargs
, seems like you could do some minimal configuration and get this --help
output for free (as well as help
subcommand).
var argv = require("yargs")
.usage("$0 [options]")
.options({
"tag-from": {
type: "string",
desc: "A git tag that determines the lower bound of the range of commits (defaults to last available)"
},
"tag-to": {
type: "string",
desc: "A git tag that determines the upper bound of the range of commits"
}
})
.help()
.argv;
Given the powers of yargs, you might even consider the tagFrom and tagTo to be positional arguments rather than flags, given that it really only makes sense to specify them in the order one might expect when specifying the range in git (${fromTag}..${toTag}
).
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.
Ah that's perfect! For some reasons I didn't go through the entire readme they have. I'll change that, thanks!
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.
It is a ridiculously long README, that is true. :)
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.
Done ecb8520
The positional arguments as far as I can see can only be used for a command
which in our case is not necessary.
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.
@evocateur so I tried the "positional arguments approach". It's kind of a workaround because yargs
supports that only for commands and because if we want them to be <fromTag>..<toTag>
it's considered as 1 argument (because of the ..
) and we need to do some parsing anyway.
Those will be the changes. I'll keep them stashed locally until we decide whether to go with that or not.
diff --git cli.js cli.js
index 841e66a..ae2d5c0 100755
--- cli.js
+++ cli.js
@@ -1,39 +1,57 @@
#!/usr/bin/env node
-
var chalk = require("chalk");
var Changelog = require(".").Changelog;
var ConfigurationError = require(".").ConfigurationError;
var argv = require("yargs")
- .usage("Usage: lerna-changelog [options]")
- .options({
- "tag-from": {
- type: "string",
- desc: "A git tag that determines the lower bound of the range of commits (defaults to last available)"
- },
- "tag-to": {
- type: "string",
- desc: "A git tag that determines the upper bound of the range of commits"
- }
- })
+ .usage("Usage: lerna-changelog <fromTag>..<toTag>")
.example(
"lerna-changelog",
"create a changelog for the changes after the latest available tag"
)
.example(
- "lerna-changelog --tag-from 0.1.0 --tag-to 0.3.0",
+ "lerna-changelog 0.1.0",
+ "create a changelog for the changes after the given tag"
+ )
+ .example(
+ "lerna-changelog 0.1.0..0.3.0",
"create a changelog for the changes in all tags within the given range"
)
.version()
.help()
.argv;
+var fromTag;
+var toTag;
+var args = argv._;
+if (args.length > 1) {
+ console.error(
+ chalk.red("Invalid arguments, must be in format `<fromTag>..<toTag>`")
+ );
+ process.exit(1);
+} else if (args.length === 1) {
+ var indexOfSeparator = args[0].indexOf("..");
+ if (indexOfSeparator === 0) {
+ console.error(
+ chalk.red("The lower bound tag is required `<fromTag>..<toTag>`")
+ );
+ process.exit(1);
+ } else if (indexOfSeparator === -1) {
+ fromTag = args[0];
+ } else {
+ var parts = args[0].split("..");
+ fromTag = parts[0];
+ toTag = parts[1];
+ }
+}
+
+var options = { fromTag: fromTag, toTag: toTag };
try {
- console.log((new Changelog(argv)).createMarkdown());
+ console.log(new Changelog(options).createMarkdown());
} catch (e) {
if (e instanceof ConfigurationError) {
console.log(chalk.red(e.message));
} else {
- throw (e);
+ throw e;
}
}
This is awesome btw - although it's kind of feels like a rewrite all the code I had was pretty messy. It's difficult to really review but since there's tests that's all that matters in a sense 😄 . After all just a changelog generator |
Very cool 👍 |
@hzoo @gigabo @evocateur sorry guys, I know we are all busy with more important stuff, just wondering if you got a chance to have another look at this. Thanks ;) |
I think it's good we can continue to make new prs to make it better. Difficult to review otherwise. |
#### Committers: 1 | ||
- Han Solo ([han-solo](https://github.com/han-solo)) | ||
|
||
|
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.
Non-blocking: wondering why there's 2 new lines here (unless we want that)
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 thought it's a bit better to distinguish between tags / releases. If you think it's not important I can put only 1 break like back.
@@ -0,0 +1,64 @@ | |||
exports[`createMarkdown outputs correct changelog 1`] = ` | |||
" |
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.
Is the "
here intentional + the escaped quotes below or is that just part of the snapshot?
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.
Yeah, it's part of the snapshot.
describe('hello', () => {
it('world', () => {
expect('hello\nworld').toMatchSnapshot()
})
})
exports[`foo bar 1`] = `
"hello
world"
`;
@@ -7,9 +7,10 @@ | |||
"lerna-changelog": "cli.js" | |||
}, | |||
"scripts": { | |||
"build": "babel src -d lib", | |||
"build": "npm run clean && babel src --out-dir lib --ignore src/__mocks__/GithubAPI.js,src/__mocks__/Changelog.js,src/__mocks__/ApiDataCache.js,src/__mocks__/execSync.js", |
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.
Wouldn't --ignore src/__mocks__
be sufficient here?
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.
True, thanks 😄
## Unreleased (2099-01-01) | ||
|
||
#### :rocket: New Feature | ||
* \`the-force-awakens\`, \`rogue-one\` |
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 we want it to be outputting \`
in the changelog if that's possible?
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.
It's just what the snapshot output looks like, the normal output doesn't have the escapes.
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.
Oh ok 😄
lgtm just wondering about the escaped quotes in the changelog output |
From master on Babel
## Unreleased (2017-01-26)
🐛 Bug Fix
📝 Documentation
🏠 Internal
Committers: 4
|
One last thing that I forgot guys (see 0408bcb): in case there are multiple tags referenced by the same commit, we should group each one of them separately (each one will have the same list of commits). Example: $ git log --oneline --pretty="%h;%D;%s;%cd" --date=short v0.1.0..
a0000004;tag: a-new-hope@4.0.0, tag: empire-strikes-back@5.0.0, tag: return-of-the-jedi@6.0.0;chore(release): releasing component;1977-05-25
a0000003;;Merge pull request #1 from star-wars;1977-05-25
a0000002;tag: v0.1.0;chore(release): releasing component;1966-01-01
a0000001;;fix: some random fix which will be ignored;1966-01-01 Alternatively such multiple tags are listed in the same group of commits, but I find it kind of wrong. |
On the other hand, if I merge different kind of PRs referring to different packages (with different changes), then do a release round, lerna will publish all different packages and their related tags. If we generate the changelog at this point in time, we'll result having this "huge" group of tags containing all sorts of commits, mostly unrelated to each other, which is kind of confusing. Oh man, I'm getting really confused myself now with how exactly the publishing / tagging should work for the different packages 😵 Any idea how we should approach this? |
Yes! I think for both modes we'd want to bundle up the changelog per release rather than by tag. So need to differentiate between multiple tag releases at once in independent mode vs just trying to print out previous versions (which shouldn't happen that often really) |
Can you give some examples how you would expect this to be? |
Thinking about cra (https://github.com/facebookincubator/create-react-app/releases) - the way lerna works currently is that it makes a tag for each pkg. I mean maybe you want a separate heading for each release/pkg but I don't know. I'm ok with this for now? We can just make it a major version and do some patches btw please join our slack if you haven't already! https://slack.lernajs.io/ |
We can always push out some patches Feel free to make issues for the things we didn't get to |
@emmenko trying to figure out how to run |
Thanks so much for this awesome work btw! (Wanted to get a release out but it made sense to try out a changelog on this repo since ironically it doesn't have one itself) |
Glad I could help somehow! Hope to figure out soon this release/tag thingy. 🤔 |
@emmenko can you make issues for the things you missed before? |
@hzoo most of the stuff has been already done (eslint, travis, etc). Should be fine then, if I think of something else I'll open an issue |
Closes #17, #19, #25
Summary
This PR mainly introduces a way of querying commits for a custom tags range (at the moment only commits after the last available tag are taken into consideration) and to group them by tag.
Description
To be able to query the commits for a custom range of tags, there are 2 options that we can use:
Slightly different approach by using "positional arguments"
It's kind of a workaround because `yargs` supports that only for _commands_ and because if we want them to be `..` it's considered as 1 argument (because of the `..`) and we need to do some parsing anyway.Lerna.json options
A new option
ignoreCommitters
can be used inlerna.json
to ignore matching committers. This is particularly useful for commiters such asgreenkeeper
or any kind of bot accounts.Changelog entries grouped by tag
As briefly discussed in Be able to specify a commit range (default to last tag like before) #25, having a list of commits possibly spread across multiple tags should result in multiple changelog entries for each tag.
Empty tags (with no commits) will be discarded
Some basic tests have been added, including mocked modules.
TODO
cli.js
intobin
folder)--summary
option to print information about processed commits (if commits got discarded, etc)