-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat(sub-directory): add -s, --sub-directory
option
#32
Changes from 6 commits
e5a2dd5
6c6a7f8
c5a6f07
f241a2d
d052541
6237ca5
ce1e23a
bebb8c8
0aec59f
a82d581
4899221
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make the description |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
var Bluebird = require('bluebird'); | ||
var Fs = Bluebird.promisifyAll(require('fs')); | ||
var Path = Bluebird.promisifyAll(require('path')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're not using any async functions in |
||
|
||
var STDOUT_PATH = '-'; | ||
|
||
|
@@ -42,3 +43,26 @@ exports.writeToFile = function (path, data) { | |
} | ||
}); | ||
}; | ||
|
||
/** | ||
* Normalize the input/output file path according to a sub-directory if needed | ||
* @param {String} file - path of the input/ouput file | ||
* @param {String|null} subDirectory - an explicit path to a sub-directory if given | ||
* @returns {Promise} | ||
*/ | ||
exports.normalizeFilePath = function (file, subDirectory) { | ||
return Bluebird.resolve() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
.then(function () { | ||
if (!subDirectory) { | ||
return file; | ||
} | ||
|
||
file = Path.normalize(file); | ||
|
||
if (Path.dirname(file) === '.') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for not replying on the issue earlier, but I think the desired behavior of how 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. |
||
return Path.join(subDirectory, file); | ||
} | ||
|
||
return file; | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,8 +33,14 @@ exports.getCommits = function (options) { | |
revisions = tag ? tag + '..HEAD' : ''; | ||
} | ||
|
||
var gitLogCommand = 'git log -E --format=' + FORMAT + ' ' + revisions; | ||
|
||
if (options.subDirectory) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like if you do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The combination of using default value for 'subDirectory' and discarding all the explicit path handling - make this PR way simpler 😅 |
||
gitLogCommand += ' -- ' + options.subDirectory; | ||
} | ||
|
||
return CP.execAsync( | ||
'git log -E --format=' + FORMAT + ' ' + revisions, | ||
gitLogCommand, | ||
{ | ||
maxBuffer: Number.MAX_SAFE_INTEGER | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you align the equal signs within this block of |
||
var README = Path.resolve(__dirname, '../README.md'); | ||
var STDOUT_FD = 1; | ||
|
@@ -79,4 +80,71 @@ describe('file', function () { | |
|
||
}); | ||
|
||
describe('normalizePath', function () { | ||
|
||
it('default output, no sub-directory given => CHANGELOG.md in current directory', function () { | ||
return File.normalizeFilePath(DEFAULT_FILE) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(DEFAULT_FILE); | ||
}); | ||
}); | ||
|
||
it('default name, with sub-directory given => CHANGELOG.md in sub directory', function () { | ||
var subDirectory = 'subdirectory'; | ||
|
||
return File.normalizeFilePath(DEFAULT_FILE, subDirectory) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(Path.join(subDirectory, DEFAULT_FILE)); | ||
}); | ||
}); | ||
|
||
it('explicit name, no sub-directory given => explicit name in current directory', function () { | ||
var someFile = 'SOMEFILE.md'; | ||
|
||
return File.normalizeFilePath(someFile) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(someFile); | ||
}); | ||
}); | ||
|
||
it('explicit name, with sub-directory given => explicit name in sub directory', function () { | ||
var subDirectory = 'subdirectory'; | ||
var someFile = 'SOMEFILE.md'; | ||
|
||
return File.normalizeFilePath(someFile, subDirectory) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(Path.join(subDirectory, someFile)); | ||
}); | ||
}); | ||
|
||
it('default name, with deep-sub-directory given => CHANGELOG.md in deep-sub directory', function () { | ||
var subDirectory = Path.join('path', 'to', 'subdirectory'); | ||
|
||
return File.normalizeFilePath(DEFAULT_FILE, subDirectory) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(Path.join(subDirectory, DEFAULT_FILE)); | ||
}); | ||
}); | ||
|
||
it('explicit output path, without sub-directory given => explicit output path', function () { | ||
var explicitPath = Path.join('path', 'to', DEFAULT_FILE); | ||
|
||
return File.normalizeFilePath(explicitPath) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(explicitPath); | ||
}); | ||
}); | ||
|
||
it('explicit output path, with sub-directory given => explicit output path', function () { | ||
var explicitPath = Path.join('path', 'to', DEFAULT_FILE); | ||
var subDirectory = Path.join('some', 'other', 'directory'); | ||
|
||
return File.normalizeFilePath(explicitPath, subDirectory) | ||
.then(function (normalizedPath) { | ||
Expect(normalizedPath).to.eql(explicitPath); | ||
}); | ||
}); | ||
|
||
}); | ||
|
||
}); |
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.
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?