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

New CLI options, custom tags range and tests. #31

Merged
merged 13 commits into from Jan 29, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .babelrc
@@ -1,3 +1,4 @@
{
"presets": ["es2015"]
"presets": ["es2015"],
"plugins": ["transform-object-rest-spread"]
}
2 changes: 2 additions & 0 deletions .eslintrc
Expand Up @@ -35,6 +35,8 @@
"indent": ["error", 2]
},
"env": {
"es6": true,
"jest": true,
Copy link
Contributor

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

"node": true
}
}
2 changes: 2 additions & 0 deletions .gitignore
@@ -1,3 +1,5 @@
lib
node_modules
*.swp
coverage
.changelog
1 change: 1 addition & 0 deletions .npmignore
@@ -1 +1,2 @@
src
**/__mocks__/**
14 changes: 14 additions & 0 deletions README.md
Expand Up @@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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


## CLI

```bash
$ lerna-changelog

Usage: lerna-changelog [options]


Options:
--tagFrom <tag> define a custom tag to determine the lower bound of the range of commits (default: last available git tag)
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

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 👍

--tagTo <tag> define a custom tag to determine the upper bound of the range of commits
```

[lerna-homepage]: https://lernajs.io
[hzoo-profile]: https://github.com/hzoo
Expand Down
21 changes: 17 additions & 4 deletions cli.js
@@ -1,13 +1,26 @@
#!/usr/bin/env node

var chalk = require("chalk");
var lib = require(".");
var argv = require("yargs").argv;
Copy link
Contributor

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

Copy link
Member

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).

var Changelog = require(".").Changelog;
var ConfigurationError = require(".").ConfigurationError;

var Changelog = lib.Changelog;
var ConfigurationError = lib.ConfigurationError;
if (argv.help) {
Copy link
Member

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}).

Copy link
Contributor Author

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!

Copy link
Member

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. :)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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;
   }
 }

console.log(
"\n" +
" Usage: lerna-changelog [options]" +
"\n\n\n" +
" Options:" +
"\n" +
" --tagFrom <tag> define a custom tag to determine the lower bound of the range of commits (default: last available git tag)" +
"\n" +
" --tagTo <tag> define a custom tag to determine the upper bound of the range of commits"
);
process.exit(0);
}

try {
console.log((new Changelog()).createMarkdown());
console.log((new Changelog(argv)).createMarkdown());
} catch (e) {
if (e instanceof ConfigurationError) {
console.log(chalk.red(e.message));
Expand Down
28 changes: 21 additions & 7 deletions package.json
Expand Up @@ -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",
Copy link
Contributor Author

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

Copy link
Contributor

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)

Copy link
Contributor Author

@emmenko emmenko Jan 16, 2017

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

Copy link
Contributor

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

Copy link

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.

Copy link
Contributor Author

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!

Copy link

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.

"clean": "rimraf lib",
"test": "eslint index.js cli.js src/",
"lint": "eslint index.js cli.js src/",
Copy link
Contributor

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

"test": "jest",
"prepublish": "npm run build"
},
"repository": {
Expand All @@ -27,16 +28,29 @@
},
"homepage": "https://github.com/lerna/lerna-changelog#readme",
"devDependencies": {
"babel-cli": "^6.9.0",
"babel-preset-es2015": "^6.9.0",
"eslint": "^2.10.2",
"rimraf": "^2.5.2"
"babel-cli": "^6.18.0",
"babel-eslint": "^7.1.1",
"babel-jest": "^18.0.0",
"babel-plugin-transform-object-rest-spread": "6.20.2",
"babel-preset-es2015": "^6.18.0",
"eslint": "^3.13.1",
"jest": "^18.1.0",
"lerna": "^2.0.0-beta.32",
"rimraf": "^2.5.4"
},
"peerDependencies": {
"lerna": "^2.0.0-beta.8"
},
"dependencies": {
"chalk": "^1.1.3",
"mkdirp": "^0.5.1"
"mkdirp": "^0.5.1",
"yargs": "^6.6.0"
},
"jest": {
"globals": {
"process.env": {
"NODE_ENV": "test"
}
}
}
}
16 changes: 9 additions & 7 deletions src/ApiDataCache.js
Expand Up @@ -4,30 +4,32 @@ import mkdirp from "mkdirp";
import ConfigurationError from "./ConfigurationError";

export default class ApiDataCache {
constructor(host, {rootPath, cacheDir}) {
constructor(host, { rootPath, cacheDir }) {
this.host = host;
const dir = this.dir = cacheDir && path.join(rootPath, cacheDir, host);

if (dir) {
try {
mkdirp.sync(dir);
} catch (e) {
throw new ConfigurationError(`Can't use cacheDir "${cacheDir}" (${e.message})`);
throw new ConfigurationError(
`Can't use cacheDir "${cacheDir}" (${e.message})`
);
}
}
}

get(type, key) {
if (!this.dir) return;
if (!this.dir)
return;
try {
return fs.readFileSync(this.fn(type, key), "utf-8");
} catch (e) {
// Pass.
}
} catch (e) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this violating some linter rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, I just used auto-formatting from prettier.
Happy to revert it if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. That sort of thing is generally better to do in a dedicated PR. Makes it easier to pick out relevant changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is similar to the eslint comment I made above - we should just use the babel eslint config or w.e separately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, my bad. I usually also prefer to do it in a separate PR. I'll revert those changes, no problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted extra changes here a8ef94b

}

set(type, key, data) {
if (!this.dir) return;
if (!this.dir)
return;
return fs.writeFileSync(this.fn(type, key), data);
}

Expand Down