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
Moves spinner and logError to utils #140
Conversation
Signed-off-by: campionfellin <campionfellin@gmail.com>
src/utils.ts
Outdated
@@ -5,6 +5,8 @@ const splitLines = require('split-lines'); | |||
import * as fs from 'fs'; | |||
const dotf = require('dotf'); | |||
const read = require('read-file'); | |||
import { Spinner } from 'cli-spinner'; | |||
import { ERROR } from '../index.js'; |
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'm not a huge fan of importing things into Utils from the main script. I'd rather have ERROR
in src/utils.ts
or in src/commands
and import from there if needed.
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 creating a cyclic dependency.
Please move the ERRORs into utils
.
Pull Request Test Coverage Report for Build 104
💛 - Coveralls |
2 similar comments
Pull Request Test Coverage Report for Build 104
💛 - Coveralls |
Pull Request Test Coverage Report for Build 104
💛 - Coveralls |
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'd rather export LOG/ERROR in utils than index.
LGTM after addressing comments.
src/utils.ts
Outdated
@@ -5,6 +5,8 @@ const splitLines = require('split-lines'); | |||
import * as fs from 'fs'; | |||
const dotf = require('dotf'); | |||
const read = require('read-file'); | |||
import { Spinner } from 'cli-spinner'; | |||
import { ERROR } from '../index.js'; |
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 creating a cyclic dependency.
Please move the ERRORs into utils
.
src/utils.ts
Outdated
@@ -72,4 +74,32 @@ export const DOTFILE = { | |||
// See `login`: Stores { accessToken, refreshToken } | |||
RC: dotf(DOT.RC.DIR, DOT.RC.NAME), | |||
RC_LOCAL: dotf(DOT.RC.LOCAL_DIR, DOT.RC.NAME), | |||
}; | |||
|
|||
// Utils |
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.
Remove // Utils
, the filename now indicates this.
index.ts
Outdated
@@ -110,7 +109,7 @@ const LOG = { | |||
}; | |||
|
|||
// Error messages (some errors take required params) | |||
const ERROR = { | |||
export const ERROR = { |
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.
Do not export ERROR
in index.ts
.
I think LOG and ERROR can be exported in utils.ts
despite what I said earlier.
Signed-off-by: campionfellin <campionfellin@gmail.com>
Merge conflict. |
Conflict resolved. |
Continuing work on #133
Signed-off-by: campionfellin campionfellin@gmail.com
npm run test
succeeds.