feat(sub-directory): add `-s, --sub-directory` option #32

altJake · 2017-12-07T01:25:14Z

Add an option to generate a changelog for a specific sub-directory in the repository, using the git log -- <path> command ability.

Also add a reconciling/normalizing of the input/output file path to handle the read/write in a sub-directory, when it is given.

closes #13

robinjoseph08

Thanks for contributing, @altJake! Sorry for the delay in the review, but hopefully we can get this merged soon. I left some comments, but if you have any questions, feel free to ask!

robinjoseph08 · 2017-12-16T21:26:00Z

lib/file.js

+
+ file = Path.normalize(file);
+
+ if (Path.dirname(file) === '.') {


Sorry for not replying on the issue earlier, but I think the desired behavior of how -f and -s will work together will be to always have it be relative from the subdirectory. I think that will give the most options for where the file should go. For example, in the current implementation, I'm not sure there's any way to write the file to root directory of the repo (I tried CHANGELOG.md, ../CHANGELOG.md, ./../CHANGELOG.md).

And since this is a completely new feature, we don't have to worry about backwards compatibility as long as we don't affect the current functionality. So we should pick what makes the most sense and provides the greatest control.

robinjoseph08 · 2017-12-16T21:27:21Z

lib/file.js

+ * @returns {Promise}
+ */
+exports.normalizeFilePath = function (file, subDirectory) {
+ return Bluebird.resolve()


It doesn't look like anything in this function is actually asynchronous so you don't have to make it return a promise. It should make the implementation a bit simpler.

robinjoseph08 · 2017-12-16T21:27:55Z

lib/file.js

@@ -2,6 +2,7 @@

 var Bluebird = require('bluebird');
 var Fs = Bluebird.promisifyAll(require('fs'));
+var Path = Bluebird.promisifyAll(require('path'));


We're not using any async functions in path so we don't have to promisify it.

robinjoseph08 · 2017-12-16T21:36:20Z

lib/git.js

@@ -33,8 +33,14 @@ exports.getCommits = function (options) {
 revisions = tag ? tag + '..HEAD' : '';
 }

+ var gitLogCommand = 'git log -E --format=' + FORMAT + ' ' + revisions;
+
+ if (options.subDirectory) {


It looks like if you do git log -- ., it behaves the same way as git log, so it might simplify the logic if you make the default value of subDirectory . in lib/cli.js.

The combination of using default value for 'subDirectory' and discarding all the explicit path handling - make this PR way simpler 😅
Liked it way better though 👍

robinjoseph08 · 2017-12-16T21:37:07Z

test/file.test.js

@@ -7,6 +7,7 @@ var Sinon = require('sinon');

 var File = require('../lib/file');

+var DEFAULT_FILE = './CHANGELOG.md';
 var NONEXISTENT = Path.resolve(__dirname, '../fake-file.txt');


Can you align the equal signs within this block of vars?

robinjoseph08 · 2017-12-16T21:43:18Z

lib/cli.js

@@ -17,4 +17,5 @@ module.exports = CLI
 .option('-t, --tag <range>', 'generate from specific tag or range (e.g. v1.2.3 or v1.2.3..v1.2.4)')
 .option('-x, --exclude <types>', 'exclude selected commit types (comma separated)', list)
 .option('-f, --file [file]', 'file to write to, defaults to ./CHANGELOG.md, use - for stdout', './CHANGELOG.md')
- .option('-u, --repo-url [url]', 'specify the repo URL for commit links, defaults to checking the package.json');
+ .option('-u, --repo-url [url]', 'specify the repo URL for commit links, defaults to checking the package.json')
+ .option('-s, --sub-directory <path>', 'specify an explicit relatvie path to a sub-directory');


Can you make the description specify a path to be passed into git log? And then maybe add a subsection within ## Usage in the README titled ### Sub-directory under ### Recommended to explain the nuances with how --file is relative from --sub-directory since it is a bit nuanced?

robinjoseph08 · 2017-12-16T21:44:37Z

README.md

@@ -62,6 +62,7 @@ $ changelog -h
 -x, --exclude <types> exclude selected commit types (comma separated)
 -f, --file [file] file to write to, defaults to ./CHANGELOG.md, use - for stdout
 -u, --repo-url [url] specify the repo URL for commit links, defaults to checking the package.json
+ -s, --sub-directory <path> specify an explicit relatvie path to a sub-directory


Looks like the alignment here is also a bit off. Can you generate the help and just replace the whole help text to make sure it's exactly as it would be outputted?

- update ouput of help command - add section to clarify sub-directory flag's usage nuances

altJake · 2017-12-17T23:05:58Z

Hope it looks better and cleaner now.
@robinjoseph08 let me know if you feel like re-wording or an example are needed for the new section of the usage

altJake · 2018-01-29T00:42:06Z

Hi @robinjoseph08 !

Just bump it, probably has been forgotten (also by me)
Looking forward to hear from you

robinjoseph08

Hey @altJake! Yeah, looks like it slipped through the cracks, sorry again about that.

It's all looking really good, the last thing we need to make sure we account for is when using --sub-directory and --file - at the same time. Should be easy enough to fix by deferring the joining of the sub-directory and the file paths.

Let me know if you have any questions! Thanks again for your help!

robinjoseph08 · 2018-02-02T21:37:43Z

bin/generate


 var CLI = require('../lib/cli');
 var Changelog = require('../lib');
 var File = require('../lib/file');

 CLI.parse(process.argv);

+var filePath = Path.join(CLI.subDirectory, CLI.file);


Hmm, I'm not sure if we can join the paths together here since some people might be passing in - for stdout. We might need to pass CLI.subDirectory through to the two functions. We also should probably be checking for - explicitly in File.readIfExists, but you don't have to do that in this PR.

robinjoseph08 · 2018-03-28T17:47:27Z

@altJake bump!

dmlittle · 2018-05-09T22:46:51Z

@altJake bump!

dmlittle · 2018-07-13T21:59:00Z

Hey @altJake, I'll be closing this PR as there hasn't been any activity for a while. If you want to revisit this issue in the future feel free to comment and we can re-open this PR.

altJake added 6 commits December 7, 2017 02:15

Add sub-directory option for CLI

e5a2dd5

Add git log command filtering when sub-directory is specified

6c6a7f8

Add output file path normalization when sub-directory option is used

c5a6f07

Add path normalization as prior step to changelog generation

f241a2d

Add tests to cover new functionalities

d052541

Update README.md

6237ca5

robinjoseph08 self-assigned this Dec 13, 2017

robinjoseph08 suggested changes Dec 16, 2017

View reviewed changes

robinjoseph08 assigned altJake and unassigned robinjoseph08 Dec 16, 2017

altJake added 5 commits December 17, 2017 23:37

Update argument text and add defaulting to current folder ('.')

ce1e23a

Exchange path normalization with simple Path.join(...) call

bebb8c8

Clean up all normalizeFilePath function and its tests

0aec59f

Simplify the git log command generation logic and remove redundant test

a82d581

Update README file:

4899221

- update ouput of help command - add section to clarify sub-directory flag's usage nuances

altJake force-pushed the master branch from 52f2ec4 to 4899221 Compare December 17, 2017 23:03

robinjoseph08 suggested changes Feb 2, 2018

View reviewed changes

dmlittle closed this Jul 13, 2018

picheli20 mentioned this pull request Nov 7, 2018

Sub directory #47

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sub-directory): add `-s, --sub-directory` option #32

feat(sub-directory): add `-s, --sub-directory` option #32

altJake commented Dec 7, 2017

robinjoseph08 left a comment

robinjoseph08 Dec 16, 2017

robinjoseph08 Dec 16, 2017

robinjoseph08 Dec 16, 2017

robinjoseph08 Dec 16, 2017

altJake Dec 17, 2017

robinjoseph08 Dec 16, 2017

robinjoseph08 Dec 16, 2017

robinjoseph08 Dec 16, 2017

altJake commented Dec 17, 2017 •

edited

Loading

altJake commented Jan 29, 2018

robinjoseph08 left a comment

robinjoseph08 Feb 2, 2018 •

edited

Loading

robinjoseph08 commented Mar 28, 2018

dmlittle commented May 9, 2018

dmlittle commented Jul 13, 2018


		file = Path.normalize(file);

		if (Path.dirname(file) === '.') {

feat(sub-directory): add -s, --sub-directory option #32

feat(sub-directory): add -s, --sub-directory option #32

Conversation

altJake commented Dec 7, 2017

robinjoseph08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altJake commented Dec 17, 2017 • edited Loading

altJake commented Jan 29, 2018

robinjoseph08 left a comment

Choose a reason for hiding this comment

robinjoseph08 Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

robinjoseph08 commented Mar 28, 2018

dmlittle commented May 9, 2018

dmlittle commented Jul 13, 2018

feat(sub-directory): add `-s, --sub-directory` option #32

feat(sub-directory): add `-s, --sub-directory` option #32

altJake commented Dec 17, 2017 •

edited

Loading

robinjoseph08 Feb 2, 2018 •

edited

Loading