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
MAINT: Updated helptext to display the correct command #216
Conversation
q2cli/builtin/tools.py
Outdated
@@ -274,7 +274,7 @@ def _load_metadata(path): | |||
@tools.command(short_help='View a QIIME 2 Visualization.', | |||
help="Displays a QIIME 2 Visualization until the command " | |||
"exits. To open a QIIME 2 Visualization so it can be " | |||
"used after the command exits, use 'qiime extract'.", | |||
"used after the command exits, use 'qiime tools view --help'.", |
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.
New helptext needs a lil tweak. Take a quick look at the expected behavior
section of the original issue. 😉
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.
Thanks for pulling this PR together, @Davrod29!
One quick change (requested inline), and we should be ready to roll. 👍
Let's talk linting and continuous integration before you push your next changes? Both good toolboxes we can put to use here.
Commit that should include liting within dev environment
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.
Lookin' good @Davrod29! I have a few incredibly minor comments, inline. Thanks!
q2cli/builtin/tools.py
Outdated
@@ -16,7 +16,6 @@ | |||
|
|||
_COMBO_METAVAR = 'ARTIFACT/VISUALIZATION' | |||
|
|||
|
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.
I like to try and keep my diffs as minimal as possible - I think it helps future readers of the code (especially when they are using a tool like git). A "no-op" change like this can create noise (in my opinion). Can you undo the deletion of this line?
q2cli/builtin/tools.py
Outdated
@@ -404,7 +403,7 @@ def validate(path, level): | |||
result.validate(level) | |||
except qiime2.plugin.ValidationError as e: | |||
header = 'Result %s does not appear to be valid at level=%s:' % ( | |||
path, level) | |||
path, level) |
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.
Was this change necessary for the overall goal of the PR? If not, maybe we can undo the change (for the reasons given above). Sorry, don't mean to be pedantic, thanks for bearing with me!
Hey Matt,
Sorry for so many changes and commits that have been made for this issue.
What I thought would be an easy fix has turned into a learning experience
for me. I will review the latest change for this PR and adjust accordingly.
Thanks!
Best,
David R.
…On Thu, Jun 6, 2019, 5:40 PM Matthew Dillon ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Lookin' good @Davrod29 <https://github.com/Davrod29>! I have a few
incredibly minor comments, inline. Thanks!
------------------------------
In q2cli/builtin/tools.py
<#216 (comment)>:
> @@ -16,7 +16,6 @@
_COMBO_METAVAR = 'ARTIFACT/VISUALIZATION'
-
I like to try and keep my diffs as minimal as possible - I think it helps
future readers of the code (especially when they are using a tool like
git). A "no-op" change like this can create noise (in my opinion). Can you
undo the deletion of this line?
------------------------------
In q2cli/builtin/tools.py
<#216 (comment)>:
> @@ -404,7 +403,7 @@ def validate(path, level):
result.validate(level)
except qiime2.plugin.ValidationError as e:
header = 'Result %s does not appear to be valid at level=%s:' % (
- path, level)
+ path, level)
Was this change necessary for the overall goal of the PR? If not, maybe we
can undo the change (for the reasons given above). Sorry, don't mean to be
pedantic, thanks for bearing with me!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#216?email_source=notifications&email_token=AHK33CKIWFPXQAV4HBYMXLTPZGU63A5CNFSM4HUJP3GKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB23QL6Q#pullrequestreview-246875642>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHK33CN5IMKOKMXCFBODHKTPZGU63ANCNFSM4HUJP3GA>
.
|
No worries @Davrod29, and certainly no need to apologize at all! There are a lot of new things coming your way right now, I feel bad about tossing one more thing ™️ at you, but, I know you've got this! 🍾 |
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.
Thanks @Davrod29, @ChrisKeefe will practice merging this (if it works).
This pull request is responsible for prompting the users to use the appropriate command. The previous command was not correct and prompted users to use a nonexistent command closes #215