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

refactor cli code #1785

Merged
merged 13 commits into from
Feb 4, 2021
Merged

refactor cli code #1785

merged 13 commits into from
Feb 4, 2021

Conversation

daemon1024
Copy link
Member

@daemon1024 daemon1024 commented Jan 2, 2021

Ref #1747

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm run test-all
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
  • Insert-step functionality is working correct as expected.

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!

@daemon1024 daemon1024 requested a review from a team as a code owner January 2, 2021 16:34
@gitpod-io
Copy link

gitpod-io bot commented Jan 2, 2021

@daemon1024 daemon1024 mentioned this pull request Jan 2, 2021
7 tasks
test/cli/templates/cli-test.js Outdated Show resolved Hide resolved
'--save-sequence [string]',
'Name space separated with Stringified sequence'
)
.option(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the only options? What if they are changed in the future? Can it be dynamically loaded from the sequencer file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to look into it. It seems like the options are manually defined in index.js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O:+1:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does seem like we're almost rebuilding the CLI in order to test the CLI here. Maybe we need cliTest() to accept an argument and use it in each test with just cliTest('--save-sequence [string]') -- so that we can see the scope of each test? Or something like that...

test/cli/templates/cli-test.js Outdated Show resolved Hide resolved
@daemon1024
Copy link
Member Author

I refactored the entire program into another file. Hopefully this makes more sense and will make it easy to test, since we will get exactly the output that cli would do and no need to mimic them repetitively.

cc @jywarren @harshkhandeparkar

@daemon1024
Copy link
Member Author

I will revert the commit, if this is out of scope of this PR, or may need to retitle the PR.

@harshkhandeparkar
Copy link
Member

Can we put it inside src/ so that there is only one index.js to look at, to make it less confusing?

@daemon1024 daemon1024 changed the title use template for commander instance in clitest refactor cli code Jan 27, 2021
@daemon1024
Copy link
Member Author

codeclimate failing 🤔 will have to refactor the code.

I wonder why it wasn't complaining earlier when the same code was in index.js

@daemon1024
Copy link
Member Author

So it wasn't exclusive to module.exports 😓

@harshkhandeparkar
Copy link
Member

It was probably failing before too but was ignored.

@daemon1024

This comment has been minimized.

@daemon1024
Copy link
Member Author

image
🥳

@jywarren
Copy link
Member

jywarren commented Feb 2, 2021

Oh wow! I hope to have time to review in the next few days. In the meantime, i appreciate reviews from anyone else, thank you!!!

@jywarren jywarren merged commit ea39f4a into publiclab:main Feb 4, 2021
@jywarren
Copy link
Member

jywarren commented Feb 4, 2021

Nice!!!!!! Thanks all!!! 🔥🔥🔥

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

Successfully merging this pull request may close these issues.

3 participants