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

CLI code refactor #665

Merged
merged 8 commits into from
Jan 31, 2019
Merged

CLI code refactor #665

merged 8 commits into from
Jan 31, 2019

Conversation

vibhorgupta-gh
Copy link

@vibhorgupta-gh vibhorgupta-gh commented Jan 12, 2019

Fixes #658

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 rake test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/reviewers for help, in a comment below

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

Thanks!

@vibhorgupta-gh vibhorgupta-gh changed the title Test CLI code refactor Jan 12, 2019
Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

This is great!!!

@jywarren
Copy link
Member

There's a let based error in node 4, however. I think there's an issue open but we should be able to set strict mode and resolve it. What do you think?

Great work!!!

@vibhorgupta-gh
Copy link
Author

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

@harshkhandeparkar
Copy link
Member

@jywarren we can compile the CLI code using NectarJS

@jywarren jywarren mentioned this pull request Jan 17, 2019
5 tasks
index.js Outdated
if (err) exit("Can't read file.");
});

// User must input a step. If steps exist, check that every step is a valid step.
if (!program.step || !validateSteps(program.step))
if (!program.step || !(require('./src/CliUtils').validateSteps(program.step, sequencer)))
Copy link
Member

Choose a reason for hiding this comment

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

How about requiring everything at the start of the file for consistency?

Copy link
Author

Choose a reason for hiding this comment

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

Oh yes, I didn't pay attention. Will do

index.js Show resolved Hide resolved
Copy link
Member

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

This looks good, perhaps I'd like one more review from someone, and a rebase? Thanks a lot, this is exciting!!!

@jywarren
Copy link
Member

Hmm, is Travis stuck?

Copy link

@Mridul97 Mridul97 left a comment

Choose a reason for hiding this comment

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

Looks good! :)

@vibhorgupta-gh
Copy link
Author

@Mridul97 @jywarren Travis seems to be stuck. Any ideas?

@harshkhandeparkar
Copy link
Member

@VibhorCodecianGupta try running the tests locally and see if anything pops up

@harshkhandeparkar
Copy link
Member

Maybe infinite recursion of require ?

@harshkhandeparkar
Copy link
Member

Did you try using the new CLI code locally? See if it gets stuck

@vibhorgupta-gh
Copy link
Author

@harshkhandeparkar checked and rechecked, everything's fine. #650 and #696 face similar issues

@harshkhandeparkar
Copy link
Member

Is that because node 4 support was dropped? Maybe the travis file is broken somehow

@harshkhandeparkar
Copy link
Member

Is your forked travis file counted? Or only the upstream file is counted?

@harshkhandeparkar
Copy link
Member

Oh the progress can be seen. It seems like it is stuck at upgrading system packages of some sort.

@vibhorgupta-gh
Copy link
Author

vibhorgupta-gh commented Jan 24, 2019

@jywarren @harshkhandeparkar Travis was just behaving weirdly I guess? The last two commits are just do/undo commits and the job kickstarted again. Travis passes now. I think we can merge this now?

@harshkhandeparkar
Copy link
Member

Yes I saw that. Maybe it was a travis bug of some sort.

@harshkhandeparkar
Copy link
Member

#696 is still in progress though. Maybe any commit would reset the procedure and fix this?

@vibhorgupta-gh
Copy link
Author

@harshkhandeparkar I guess that's it. Once the job kickstarts again, Travis resumes.
Also, could you review the changes?

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

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

✔️ . Please share a GIF if possible

@vibhorgupta-gh
Copy link
Author

vibhorgupta-gh commented Jan 24, 2019

Here is a screenshot. I'm on a slow internet here 😅
Do mention if a GIF is required. Tested, everything works fine

screen shot 2019-01-25 at 1 57 20 am

@harshkhandeparkar
Copy link
Member

No need of a GIF. Thanks! Looks good!

@harshkhandeparkar
Copy link
Member

I'm on a slow internet here

Same here. I'm using BSNL so no hopes for the next 💯 years. The latency is more than what it would take to compile image-sequencer.

@vibhorgupta-gh vibhorgupta-gh removed the request for review from tech4GT January 26, 2019 07:10
@vibhorgupta-gh
Copy link
Author

@jywarren I think this is a complete blob of work. Thoughts?

@jywarren jywarren merged commit d0bf9de into publiclab:main Jan 31, 2019
@jywarren
Copy link
Member

Looks good! Merging now, and great work, again!

@jywarren jywarren mentioned this pull request Feb 9, 2019
4 tasks
jywarren pushed a commit that referenced this pull request Mar 4, 2019
* fix insert step button

* add dist

* add new function

* remove console logs

* refactor

* Setup UI testing (#720)

* default sequencer ui test

* default step ui test suite

* intermediate step ui test

* preview ui test suite

* url methods test suite

* add set url params method test suite

* argument call tests

* test directory refactor

* travis fix

* CLI code refactor (#665)

* CLI refactor

* es6 rollback

* Travis fix

* syntax fix

* clustered require statements

* travis debug

* travis debug

* Added line that selects the "More modules..." after adding a step. (#713)

* Added line making module selection correct after adding a step

* Added line with appropriate module selection

* Add: Module tests (#748)

* WIP

* module testing harness

* adjustments

* Update package-lock.json

* Bump jsqr from 1.1.1 to 1.2.0 (#749)

Bumps [jsqr](https://github.com/cozmo/jsQR) from 1.1.1 to 1.2.0.
- [Release notes](https://github.com/cozmo/jsQR/releases)
- [Commits](https://github.com/cozmo/jsQR/commits)

Signed-off-by: dependabot[bot] <support@dependabot.com>

* refactor without spread syntax
jywarren pushed a commit that referenced this pull request Mar 17, 2019
* update dist

Signed-off-by: tech4GT <varun.gupta1798@gmail.com>

dist update

Revert "dist update"

This reverts commit 9ee2a98.

* fix insert step button

* add icon

* add dist

* use normal functions

* use normal functions

* add new function

* add dist

* remove console logs

* changes

* refactor

* add dist

* Bump looks-same from 5.0.2 to 6.0.0 (#729)

Bumps [looks-same](https://github.com/gemini-testing/looks-same) from 5.0.2 to 6.0.0.
- [Release notes](https://github.com/gemini-testing/looks-same/releases)
- [Changelog](https://github.com/gemini-testing/looks-same/blob/master/CHANGELOG.md)
- [Commits](gemini-testing/looks-same@v5.0.2...v6.0.0)

Signed-off-by: dependabot[bot] <support@dependabot.com>

* Setup UI testing (#720)

* default sequencer ui test

* default step ui test suite

* intermediate step ui test

* preview ui test suite

* url methods test suite

* add set url params method test suite

* argument call tests

* test directory refactor

* travis fix

* CLI code refactor (#665)

* CLI refactor

* es6 rollback

* Travis fix

* syntax fix

* clustered require statements

* travis debug

* travis debug

* Added line that selects the "More modules..." after adding a step. (#713)

* Added line making module selection correct after adding a step

* Added line with appropriate module selection

* Add: Module tests (#748)

* WIP

* module testing harness

* adjustments

* Fix choose file option and insert step functionality (#712)

* Fix choose file option

* changes

* Parse info.json to set module defaults (#650)

* changes

* changes

* changes

* changes

* changes

* changes

* Update package-lock.json

* Bump jsqr from 1.1.1 to 1.2.0 (#749)

Bumps [jsqr](https://github.com/cozmo/jsQR) from 1.1.1 to 1.2.0.
- [Release notes](https://github.com/cozmo/jsQR/releases)
- [Commits](https://github.com/cozmo/jsQR/commits)

Signed-off-by: dependabot[bot] <support@dependabot.com>

* Bump jsqr from 1.1.1 to 1.2.0 (#749)

Bumps [jsqr](https://github.com/cozmo/jsQR) from 1.1.1 to 1.2.0.
- [Release notes](https://github.com/cozmo/jsQR/releases)
- [Commits](https://github.com/cozmo/jsQR/commits)

Signed-off-by: dependabot[bot] <support@dependabot.com>

* dist

* changes

* working

* final touches

* revert mapTypes

* proper revert

* Fix arctangent

* working hysteresis

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

Successfully merging this pull request may close these issues.

4 participants