Rename load_file_list_of_signatures to load_pathlist_from_file

[keyabarve]

Fixes #1369

This PR renames the function load_file_list_of_signatures to load_pathlist_from_file in 6 files. These files are:

• src/sourmash/lca/command_summarize.py
• src/sourmash/command_sketch.py
• src/sourmash/commands.py
• src/sourmash/lca/command_classify.py
• src/sourmash/lca/command_index.py
• src/sourmash/sourmash_args.py

TODO:

• Add some Python unit tests in tests/test_sourmash.py for this function, including empty file list, badly formatted ones, ones with duplicates.

Checklist:

• Is it mergeable?
• make test Did it pass the tests?
• make coverage Is the new code covered?
• Did it change the command-line interface? Only additions are allowed
  without a major version increment. Changing file formats also requires a
  major version number increment.
• Was a spellchecker run on the source code and documentation after
  changes were made?
• Please note your ORCID in the comments. If you don't have one, you can register for one here.

[keyabarve]

Please review @ctb @luizirber

[codecov]

Codecov Report

Merging #1423 (b97be02) into latest (cff3c77) will increase coverage by 5.21%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           latest    #1423      +/-   ##
==========================================
+ Coverage   89.27%   94.49%   +5.21%     
==========================================
  Files         123       96      -27     
  Lines       18790    15176    -3614     
  Branches     1447     1447              
==========================================
- Hits        16775    14340    -2435     
+ Misses       1782      603    -1179     
  Partials      233      233              

Flag	Coverage Δ
python	94.49% <90.00%> (ø)
rust	?

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

Impacted Files	Coverage Δ
src/sourmash/lca/command_summarize.py	80.80% <0.00%> (ø)
src/sourmash/command_sketch.py	85.29% <100.00%> (ø)
src/sourmash/commands.py	82.63% <100.00%> (ø)
src/sourmash/lca/command_classify.py	78.75% <100.00%> (ø)
src/sourmash/lca/command_index.py	91.48% <100.00%> (ø)
src/sourmash/sourmash_args.py	92.93% <100.00%> (ø)
src/core/src/index/linear.rs
src/core/tests/test.rs
src/core/src/index/sbt/mhbt.rs
src/core/src/ffi/minhash.rs
... and 23 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 cff3c77...b97be02. Read the comment docs.

[ctb]

hi @keyabarve a couple things -

• first, it looks like you started from your last PRs branch, rather than the latest branch; so this needs to be updated from latest again :)
• separately, I'd suggest waiting to ask for a review after the checks above run the tests successfully...
• last but not least, please put Fixes #1369 in the pull request comment body, not the title - that's so that it's properly linked to the issue and the issue will be closed when this is merged. See comments at bottom replace notify format usage with f-strings instead (#1409) (#1418)  #1422 for more detailed instructions..

I note that @markjih is also working on this same issue, in #1382, but he didn't link it to the issue so you couldn't know that. That PR is a bit stale now, so I'm fine with you continuing here :). In general, tho, the goal is to try to avoid stepping on each other, which is why good issue/PR linking is so important!

[ctb]

please create a new issue for testing --query-from-file for sourmash lca summarize - thanks!

[ctb]

(because our current tests don't execute this, apparently!)

[keyabarve]

Will do, thanks!

[keyabarve]

Please review. @ctb @luizirber

[ctb]

hi @keya, the code changes look good. But let's work on improving the pull request title and description ;).

First, the title of the PR needs to be something that we can look at later to understand what this PR was about. This is because these remain associated with the project forever. Right now the title is "Rename issue #1369" which isn't very informative - can you describe briefly what you did?, e.g. "rename load_file_list_of_signatures to ..."?

Second, the body of the PR description at the top needs to reference relevant issues (which it does!) as well as indicating to GitHub which issues this PR will close. In this case I think this closes #1369, in which case you need to put the literal exact words "Fixes #1369" in there; then, when this PR is closed, issue #1369 will be closed automatically.

Third, the PR description should be at least somewhat informative. Yes, it's always possible to go read the code diff to figure out what was changed, but it's nice not to have to. So something like "This PR renames function XYZ to function ABC in files 1, 2, and 3." would be sufficient.

An example is here, if you'd like to see how I structure my PRs descriptions. You don't have to write a novel like I sometimes do, but in general any relevant information should be written in the PR description

Can you take a crack at updating the title and PR description, please? Thanks!

[keyabarve]

@ctb I have updated the title and PR description. Please review.
Issue #1369 also mentions something about adding some more tests. Could you please elaborate on that a little?
Thanks!

[ctb]

thanks, @keyabarve, everything but the title is good now. Before merging, could you change the actual title of the PR (currently Rename Issue #1369) to something like rename load_file_list_of_signatures to ...?

Regarding adding some unit tests, I think that's best done as a separate PR at this point. Can you create a new issue (like #1427) for that, and then you can choose to take it as a new PR if you like.

in terms of actual tests, please see this example for the kind of thing you want to do - create a list of files that's broken in various ways, and then use the actual function load_pathlist_from_file to try loading and assert that what happens is correct (an error, or something else). I would suggest just getting started with something and then I can help suggest more tests to add! The key point is that the tests should cover every explicit and implicit branch (if/else, try/except) in the function.

[ctb]

thank you!