-
Notifications
You must be signed in to change notification settings - Fork 17
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(cli): cli interface #53
Conversation
Codecov Report
@@ Coverage Diff @@
## master #53 +/- ##
===========================================
- Coverage 96.32% 85.26% -11.06%
===========================================
Files 11 14 +3
Lines 761 1140 +379
Branches 47 71 +24
===========================================
+ Hits 733 972 +239
- Misses 27 167 +140
Partials 1 1
Continue to review full report at Codecov.
|
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.
What happens if the git command is not available? We should probably have a nicer error message.
* @param {string} dir the wildcard directory containing git change, not necessarily the root git directory | ||
* @returns {string} the absolute path relative to the path that the user executed the bash command in | ||
*/ | ||
export function resolvePath(dir: string) { |
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 function doesn't seem to be adding much, any reason we shouldn't just use path.resolve wherever this is being used?
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 separated it out so that I can explicitly test that absolute paths are generated the way I want to.
I added an explicit git installation check which I didn't test since I consider that an integration test which is outside of scope. Let me know what you think! @chingor13 |
* @param dir the directory containing git changes | ||
* @returns {Promise<Changes>} the change set | ||
*/ | ||
export function getChanges(dir: string): Promise<Changes> { |
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.
export function getChanges(dir: string): Promise<Changes> { | |
export async function getChanges(dir: string): Promise<Changes> { |
Should this be marked async
?
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 necessarily. It's only required when we have an await
statement.
gitRootDir: string, | ||
gitDiffPattern: string | ||
): Promise<GitFileData> { | ||
return new Promise((resolve, reject) => { |
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.
Would async/await be easier 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.
I felt it was easier to explicitly be able to get the readfile
error for clearer error messaging. Otherwise I'd have to have nested try catch blocks if i wanted more precise error logging.
// tslint:disable:no-unused-expression | ||
// .null triggers ts-lint failure, but is valid chai | ||
// .true triggers ts-lint failure, but is valid chai | ||
describe('git directory diff output to git file data + content', () => { |
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.
nit: usually the describe block shows the function name under test
describe('git directory diff output to git file data + content', () => { | |
describe('getGitFileData', () => { |
* @returns {string} the absolute path relative to the path that the user executed the bash command in | ||
*/ | ||
export function resolvePath(dir: string) { | ||
const absoluteDir = path.resolve(process.cwd(), dir); |
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 this is only used for the CLI then implicitly depending on the cwd may be ok. If this will be included later in the core (which I could potentially see), we instead want the relative root to be explicit (and provided by the caller - e.g. the 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.
this is explicitly for the CLI. I've intended the bin directory to be strictly for the CLI. If we want to do work for the framework-core it will not be handled here. It is a small function so if we wanted to have similar behaviour in the framework-core it might make more sense to duplicate the work.
Co-authored-by: Jeff Ching <chingor@google.com>
Create a CLI interface to interact with the framework-core
It:
Closes #35
Redo of #51