-
Notifications
You must be signed in to change notification settings - Fork 7
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
Refactor/refactor code to enable testing #52
Conversation
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.
Awesome refactor, @neodmy !
I've added some comments and questions.
Apart from that, perhaps we need to discuss where to place some functions. The code as you left it after the refactor looks very clean and organized. However, there is already a utils
file, and we have methods both in the runner and utils files. So perhaps we can move all to one single file. What do you think?
Sure 👍 |
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.
LGTM!
297b954
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.
LGTM!
Main changes
In order to enable unit and end-to-end testing, I propose to split the code into several parts:
index.js
has been simplified and acts as a dependency injector and the only point for error handling:yargs
middleware
to run some checks and transform certain arguments. For instance, the arguments passed tofailOn
might include a RegEx. These RegExs must be valid.licence-checker
, which is now wrapped in a promiseAdditional notes
(optional)
Context