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

Add cli interface for script adding/editing functions #6

Merged

Conversation

harshitbansal05
Copy link
Contributor

No description provided.

@harshitbansal05
Copy link
Contributor Author

@henrykironde @zhangcandrew is it fine now?

@zhangcandrew
Copy link
Member

Hey @harshitbansal05, I haven't tested it yet. Can you include screenshots of example runs of this program on your cli and add descriptions?

@harshitbansal05
Copy link
Contributor Author

@zhangcandrew @henrykironde the screenshots:

New JSON:

Screenshot 2019-06-27 at 1 34 31 PM

Edit JSON:

Screenshot 2019-06-27 at 1 36 00 PM

Delete JSON:

Screenshot 2019-06-27 at 1 36 33 PM

recipes/lib/get_opts.py Outdated Show resolved Hide resolved
recipes/lib/get_opts.py Outdated Show resolved Hide resolved
recipes/__main__.py Show resolved Hide resolved
recipes/lib/get_opts.py Outdated Show resolved Hide resolved
@zhangcandrew
Copy link
Member

Thanks Harshit, I think there are a couple minor things we should fix up before merging in, but overall, it looks in a state that will be good to merge :)

@henrykironde
Copy link
Contributor

Correct me if am wrong. Looks like the required changes are related to making the edit functionality work as expected. We could add that here or create an Issue and we get this in.

@zhangcandrew
Copy link
Member

@henrykironde, we were just talking about how if you use edit_json and change the name of the scripts, a second version of the script is created with the new name while the script with the old names still stays. I think we talked about this previously before, but I think the correct functionality should be to replace the old scripts with the new name script, leaving no trace of the old script.

@henrykironde
Copy link
Contributor

@harshitbansal05, let's try to keep track of the reviews and comments that have been requested by the reviewers.

@harshitbansal05
Copy link
Contributor Author

@henrykironde @zhangcandrew just created an issue #10 for the edit_json problem.

@harshitbansal05 harshitbansal05 force-pushed the cli-interface branch 2 times, most recently from a19f4ad to 8149aae Compare July 8, 2019 09:59
@harshitbansal05
Copy link
Contributor Author

@zhangcandrew I added more search paths for the edit_json functionality, could you please review it?

@zhangcandrew
Copy link
Member

Looks good to merge in! Thanks for fixing issues.

One last thing, can you change the os.system calls to subprocess.call methods in setup.py. Setup.py isn't working for copying the commit hook files over.

@harshitbansal05
Copy link
Contributor Author

@zhangcandrew sure. However, it did work for me.

@harshitbansal05
Copy link
Contributor Author

@zhangcandrew a polite reminder.

@henrykironde
Copy link
Contributor

Redesign the hard coded parts

@harshitbansal05
Copy link
Contributor Author

@zhangcandrew @henrykironde done.

@harshitbansal05
Copy link
Contributor Author

@zhangcandrew @henrykironde can this go in now?

@zhangcandrew
Copy link
Member

LGTM, mergin in

@zhangcandrew zhangcandrew merged commit 55a661a into weecology:retriever-recipes-dev Jul 12, 2019
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