Skip to content
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

use require() in index.js to source from /src/cli/ #658

Closed
jywarren opened this issue Jan 12, 2019 · 9 comments
Closed

use require() in index.js to source from /src/cli/ #658

jywarren opened this issue Jan 12, 2019 · 9 comments

Comments

@jywarren
Copy link
Member

The index.js file in the root directory runs the CLI interface. But the CLI code is a bit long now, and could be easier to read. Let's break it up into a few different files kept in /src/cli/ and use require() to access them from index.js. This will result in cleaner and easier to read code.

Personally I like this type of task! It can be pretty fun to tidy things up. The CLI tests should help us ensure this is done properly, too. We'd love some help!

@gitmate
Copy link

gitmate bot commented Jan 12, 2019

GitMate.io thinks the contributor most likely able to help you is @ccpandhare.

Possibly related issues are #26 (Break out longer functions into sub source files and require in), and #150 (Using GitMate).

1 similar comment
@gitmate
Copy link

gitmate bot commented Jan 12, 2019

GitMate.io thinks the contributor most likely able to help you is @ccpandhare.

Possibly related issues are #26 (Break out longer functions into sub source files and require in), and #150 (Using GitMate).

@vibhorgupta-gh
Copy link

vibhorgupta-gh commented Jan 12, 2019

@jywarren @tech4GT PR #665 addresses this.

The validateSteps and validateConfig methods were moved to CliUtils.js where the makedir method, also used in CLI code, is written.
src/cli/ contains files according to the commands that the user will give. --save-sequence has its own file, --install-module has its own file and the code handling the process of generating module outputs is has its separate file. Everything is required in index.js. Thoughts?

@vibhorgupta-gh vibhorgupta-gh mentioned this issue Jan 12, 2019
4 tasks
@vibhorgupta-gh
Copy link

@jywarren using strict mode is always good, as you mentioned in the PR, let's do it!
However, is the let based error affecting this PR too? I don't think it is...

@vibhorgupta-gh
Copy link

@jywarren also, if we'd like to fix the let issue in Travis, let's replace it with var for now, because using the strict mode requires a lot of other things to be taken care of, which I think is best to pursue in a separate issue. A new issue could be created dedicated to introducing the strict scope in js files. For simplicity though, I'd suggest merging #665 without a major overhaul

@jywarren
Copy link
Member Author

jywarren commented Jan 12, 2019 via email

@vibhorgupta-gh
Copy link

Shall I then? Replace the let with var?

@jywarren
Copy link
Member Author

jywarren commented Jan 12, 2019 via email

@vibhorgupta-gh
Copy link

@jywarren is #665 mergeable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants