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 a context manager for zipping FASTX files #77

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Conversation

clintval
Copy link
Member

@clintval clintval commented Nov 24, 2023

Helpful addition? Docs appear to render correctly on my local machine:

Screenshot 2023-11-24 at 1 04 03 PM

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

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

Comparison is base (66b2200) 92.88% compared to head (06f9a85) 93.27%.
Report is 1 commits behind head on main.

Files Patch % Lines
fgpyo/fastx/__init__.py 95.12% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   92.88%   93.27%   +0.38%     
==========================================
  Files          30       32       +2     
  Lines        3122     3331     +209     
  Branches      581      618      +37     
==========================================
+ Hits         2900     3107     +207     
- Misses        148      149       +1     
- Partials       74       75       +1     

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

Copy link
Member

@nh13 nh13 left a comment

Choose a reason for hiding this comment

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

Shall we suggest adding this to pysam to the pysam developers?

fgpyo/fastx/__init__.py Outdated Show resolved Hide resolved
fgpyo/fastx/__init__.py Outdated Show resolved Hide resolved
@clintval
Copy link
Member Author

Shall we suggest adding this to pysam to the pysam developers?

What's been your approach on this for pysam vs fgpyo? I'm up for it!

I have a few more PRs to make in pysam for all matter of wrong docs, bugs, and features I recently found.

@nh13
Copy link
Member

nh13 commented Nov 28, 2023

What's been your approach on this for pysam vs fgpyo? I'm up for it!

I ask first, and it's nice that you can point it to here (and we can even merge it!)

@clintval
Copy link
Member Author

I want to add one more defensive check to make sure the class isn't used as an Iterator outside of the context manager context (which would result in a bug). Just thought of that edge case prior to merge!

@clintval clintval assigned clintval and unassigned nh13 Nov 29, 2023
@clintval
Copy link
Member Author

@nh13 I added one more commit and feel good with the functionality.

Let me know if you'd like to re-review!

@nh13
Copy link
Member

nh13 commented Nov 30, 2023

No need for me to re review. Ship it!

@clintval clintval merged commit fb77301 into main Nov 30, 2023
6 checks passed
@clintval clintval deleted the cv_fastx_zipped branch November 30, 2023 06:13
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.

2 participants