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

[MRG] Adding brief descriptions and -h/--help text to sourmash gather, search, and compare #1735

Merged
merged 7 commits into from
Oct 3, 2021

Conversation

keyabarve
Copy link
Contributor

@keyabarve keyabarve commented Sep 2, 2021

Addresses #1557

What was done:

Commands fixed:

  • sourmash gather
  • sourmash search
  • sourmash compare

NOTE:
The descriptions for all of the sourmash sig subcommands have been addressed in issue #1612 handled by PR #1714.

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #1735 (82d93ec) into latest (c093c57) will increase coverage by 7.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1735      +/-   ##
==========================================
+ Coverage   82.72%   90.10%   +7.38%     
==========================================
  Files         114       87      -27     
  Lines       12200     8401    -3799     
  Branches     1555     1555              
==========================================
- Hits        10092     7570    -2522     
+ Misses       1850      573    -1277     
  Partials      258      258              
Flag Coverage Δ
python 90.10% <100.00%> (+<0.01%) ⬆️
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/sourmash/cli/compare.py 100.00% <100.00%> (ø)
src/sourmash/cli/gather.py 100.00% <100.00%> (ø)
src/sourmash/cli/search.py 100.00% <100.00%> (ø)
src/core/src/ffi/utils.rs
src/core/src/ffi/hyperloglog.rs
src/core/src/index/sbt/mod.rs
src/core/src/signature.rs
src/core/src/ffi/signature.rs
src/core/src/index/search.rs
src/core/src/ffi/mod.rs
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c093c57...82d93ec. Read the comment docs.

@keyabarve
Copy link
Contributor Author

@ctb I added the docstring and usage for the gather command, but when I run sourmash gather -h, it does not produce the necessary information, instead it gives an error:

Traceback (most recent call last):
  File "/Users/keya/mambaforge/envs/sourmash_dev/bin/sourmash", line 33, in <module>
    sys.exit(load_entry_point('sourmash', 'console_scripts', 'sourmash')())
  File "/Users/keya/sourmash/src/sourmash/__main__.py", line 5, in main
    args = sourmash.cli.get_parser().parse_args(arglist)
  File "/Users/keya/sourmash/src/sourmash/cli/__init__.py", line 79, in parse_args
    args = super(SourmashParser, self).parse_args(args=args, namespace=namespace)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1818, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1851, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 2042, in _parse_known_args
    positionals_end_index = consume_positionals(start_index)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 2019, in consume_positionals
    take_action(action, args)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1928, in take_action
    action(self, namespace, argument_values, option_string)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1207, in __call__
    subnamespace, arg_strings = parser.parse_known_args(arg_strings, None)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1851, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 2060, in _parse_known_args
    start_index = consume_optional(start_index)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 2000, in consume_optional
    take_action(action, args, option_string)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1928, in take_action
    action(self, namespace, argument_values, option_string)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 1092, in __call__
    parser.print_help()
  File "/Users/keya/sourmash/src/sourmash/cli/__init__.py", line 72, in print_help
    super(SourmashParser, self).print_help()
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 2548, in print_help
    self._print_message(self.format_help(), file)
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 2532, in format_help
    return formatter.format_help()
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 283, in format_help
    help = self._root_section.format_help()
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 214, in format_help
    item_help = join([func(*args) for func, args in self.items])
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 214, in <listcomp>
    item_help = join([func(*args) for func, args in self.items])
  File "/Users/keya/mambaforge/envs/sourmash_dev/lib/python3.9/argparse.py", line 300, in _format_usage
    usage = usage % dict(prog=self._prog)
ValueError: unsupported format character '`' (0x60) at index 1012

I think the error is being thrown at this line:
1.4 Mbp 11.0% 58.0% JANA01000001.1 Fusobacterium sp. OBRC...
at the % 58.0% part, but I'm not sure how to fix this.

Could you provide any hints/suggestions?

@keyabarve
Copy link
Contributor Author

@ctb I noticed that the problem was with the % sign, so I replaced all the % signs with the word "percent" since I couldn't find any other way to fix it. I'm not sure if that's the correct way though.

@keyabarve
Copy link
Contributor Author

I also noticed that the sketch dna, protein, translate commands already have the doc and usage strings.

@keyabarve
Copy link
Contributor Author

@ctb Could you please take a look at what I've written and let me know if I should change anything?

@ctb
Copy link
Contributor

ctb commented Sep 6, 2021 via email

@keyabarve
Copy link
Contributor Author

keyabarve commented Sep 9, 2021

@ctb A few comments:

  • I was able to fix the errors with the % signs.
  • I believe the descriptions for all the sourmash sig subcommands is being handled in [MRG] Upgrading command-line docs for sourmash sig subcommands #1714.
  • I have added the descriptions for the gather, search andcompare commands.
  • I noticed that for sketch dna, translate and protein, the descriptions already exist in the respective files.

Please let me know if I need to add/change anything else.

@keyabarve keyabarve changed the title [WIP] Adding brief descriptions and -h/--help text to sourmash commands [MRG] Adding brief descriptions and -h/--help text to sourmash commands Sep 10, 2021
@keyabarve
Copy link
Contributor Author

@ctb Please review.

@keyabarve
Copy link
Contributor Author

@mr-eyes Please review.

@ctb
Copy link
Contributor

ctb commented Oct 1, 2021

@mr-eyes Please review.

I think this one needs a lot of familiarity with sourmash use cases, so please wait to merge it until I review as well. But it could certainly use interim reviews from @mr-eyes and/or @hehouts :)

Copy link
Contributor

@hehouts hehouts left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@ctb
Copy link
Contributor

ctb commented Oct 3, 2021

hi folks,

I was caught a little bit by surprise when I saw that the Markdown syntax (including links) was simply included in the help text, e.g.

[Jaccard index][3] or
(if signatures are created with `-p abund`) the [angular
similarity](https://en.wikipedia.org/wiki/Cosine_similarity#Angular_distance_and_similarity).

here it's worth noting a few things -

  • [3] doesn't mean anything at all in this context - it's a numerical link reference to a link in the original doc file, doc/command-line.md.
  • Markdown syntax isn't rendered at the command line, so in the help text output on the command line we should make sure it makes sense.

I'm working on making the necessary updates, but this is a plea to think about how the user is going to experience the changes and incorporate that into changes and/or reviews. thanks :)

@ctb
Copy link
Contributor

ctb commented Oct 3, 2021

It's also worth noting that while markdown->html rendering doesn't pay attention to line breaks, that is NOT true for the text output to command line. So reformatting things so that lines present properly is also worth doing.

@ctb ctb changed the title [MRG] Adding brief descriptions and -h/--help text to sourmash commands [MRG] Adding brief descriptions and -h/--help text to sourmash gather, search, and compare Oct 3, 2021
@ctb
Copy link
Contributor

ctb commented Oct 3, 2021

Hi @keyabarve if you merge in #1747, and everything looks good, we can merge this. Thank you!

@ctb
Copy link
Contributor

ctb commented Oct 3, 2021

hi @keyabarve also -

  • please adjust the PR description at the top of this PR to be accurate and straightforward; this includes eliminating checkboxes that you don't plan on checking, changing the TODO to something like 'What we did', and referencing the right PR for sourmash sig commands, i.e. adding the issue and/or PR to '(Addressed in another issue)'.
  • reference this PR in a comment over on add brief descriptions and -h/--help text to sourmash commands #1557 stating that gather, search, and compare were handled in this PR, and that the sourmash sig commands are being handled elsewhere.

Basically any sourmash dev should be able to read the top of this PR to figure out what was done (not what you intended to do, or how it was done ;); and any sourmash dev should be able to look at #1557 and see what is left to be done and where it is being done. Thanks!

…e`. (#1747)

* reformat compare -h output

* reformat/edit gather output

* reformat gather, search
Copy link
Contributor

@ctb ctb left a comment

Choose a reason for hiding this comment

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

thank you! please merge if & when tests pass!

@keyabarve
Copy link
Contributor Author

Will do! Thanks!

@keyabarve keyabarve merged commit d91911b into latest Oct 3, 2021
@keyabarve keyabarve deleted the KB_1557 branch October 3, 2021 18:19
@ctb
Copy link
Contributor

ctb commented Oct 3, 2021

🎉

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