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

MAINT: Removed 'move_files' function from 'reads.py' #25

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

VinzentRisch
Copy link
Collaborator

  • move_files was a helper function of annotate_reads_card that moved the files that where created by the analysis, to the allele and gene sample directories
  • Not using a helper function here has the benefit of making the tests easier. And for ENH: Add new action that predicts the origin of detected ARGs. #21, additional files have to be added to the CARDAnnotationDirectoryFormat and with this setup it will be easier.

@VinzentRisch VinzentRisch requested a review from Sann5 January 9, 2024 10:52
@VinzentRisch
Copy link
Collaborator Author

Hi Santiago, Michal told me to assign PRs also to you to divide the work load better.
But don't fear this is a really small one, not like the last one.

@Sann5
Copy link

Sann5 commented Jan 9, 2024

Hey @VinzentRisch. I ❤️ small PRs ;). I'll take a look tmo.

Copy link

@Sann5 Sann5 left a comment

Choose a reason for hiding this comment

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

  • ✅ It looks good to me! I just made one style suggestion (deleting a blank line between a comet and its code). I also ran the tests locally and they all passed.

  • 👀 TLDR; If you already tried the action out with the modification go ahead and merge!

    • In the future it would be nice if your PR message contains a demo on how to run the action that you are modifying so that the reviewer can run it or play around with it, to see that nothing breaks. I would imagine that there is no issue here since you are just moving around already existing functionality.

@VinzentRisch
Copy link
Collaborator Author

Thanks :)

And yes I will always include it in the future I just thought the changes are so small that you don't have have to try to run it.

@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (94069c6) 66.74% compared to head (8f4b64f) 66.70%.

❗ Current head 8f4b64f differs from pull request most recent head 65695c1. Consider uploading reports for the commit 65695c1 to get more accurate results

Files Patch % Lines
q2_amr/card/reads.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   66.74%   66.70%   -0.04%     
==========================================
  Files          13       13              
  Lines         836      835       -1     
==========================================
- Hits          558      557       -1     
  Misses        278      278              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@VinzentRisch VinzentRisch merged commit 898241c into bokulich-lab:main Jan 11, 2024
5 checks passed
@VinzentRisch VinzentRisch deleted the remove_move_files branch January 11, 2024 10:56
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