-
Notifications
You must be signed in to change notification settings - Fork 79
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
Implement modular approach to commands. #46
Implement modular approach to commands. #46
Conversation
All the commands are now located in their corresponding files under the cmds directory.
All files under cmds directory will be checked for linting.
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.
@armansujoyan I've left a few comments which need attention. Overall looks great!
cmds/standings.js
Outdated
const updateMessage = helpers.updateMessage; | ||
const standingsHelper = helpers.standingsHelper; | ||
|
||
const footballRequest = request.defaults({ |
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.
Let's move this to separate utility file for scores
, standings
and fixtures
.
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.
You mean creating separate files for scores, fixtures and standings instead of having one helper.js right?
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.
No, let's keep one request_utils.js
which exports this object.
cmds/standings.js
Outdated
const leagueIds = require('../leagueIds'); | ||
|
||
const updateMessage = helpers.updateMessage; | ||
const standingsHelper = helpers.standingsHelper; |
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.
helpers.standingsHelper
-> helpers.standings
cmds/config.js
Outdated
obj.API_KEY = answers.API_KEY; | ||
} | ||
|
||
fs.writeFileSync(path.resolve(__dirname, 'config.json'), JSON.stringify(obj, null, 2), 'utf8'); |
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 config file should be written in root directory and not in cmds
. Please fix the same.
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.
@armansujoyan Thanks @armansujoyan! LGTM! Merging! 😄
Description of new feature, or changes
Commands are moved to their corresponding files. This modular approach will make it more clear and easy to use.
Link to YARGS
Official documentation of modular approach.
Related Issues and Discussions
Enhancement: Separate commands into modules. #41
People to notify
@manrajgrover