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.
|
| * @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.
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.
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.
| 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.
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.
Would async/await be easier here?
There was a problem hiding this comment.
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.
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.
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.
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