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

feat: Add bedrecord/interval converters #27

Merged
merged 6 commits into from
Mar 20, 2024
Merged

Conversation

msto
Copy link
Contributor

@msto msto commented Mar 8, 2024

🙏

Let me know if you'd like equivalent methods added to Interval

@msto msto requested review from jdidion and nh13 March 8, 2024 17:56
@msto msto force-pushed the ms_alternate-bed-constructors branch from 931d569 to 9808c99 Compare March 8, 2024 18:20
@msto
Copy link
Contributor Author

msto commented Mar 8, 2024

I ran into circular imports when the converters were declared on BedRecord, so I moved them to Interval.

I have a slight preference for putting them on BedRecord, but not enough to deal with the headache of restructuring the project

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 92.68293% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 91.79%. Comparing base (9bbabbe) to head (7a9729c).

Files Patch % Lines
pybedlite/bed_record.py 75.00% 1 Missing and 1 partial ⚠️
pybedlite/overlap_detector.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   91.41%   91.79%   +0.38%     
==========================================
  Files           7        8       +1     
  Lines         489      524      +35     
  Branches       86       92       +6     
==========================================
+ Hits          447      481      +34     
  Misses         26       26              
- Partials       16       17       +1     

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

pybedlite/tests/conftest.py Outdated Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Outdated Show resolved Hide resolved
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.

Minor fixups that I trust you can make. Thank-you!!!

pybedlite/overlap_detector.py Outdated Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Outdated Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
@nh13
Copy link
Member

nh13 commented Mar 8, 2024

@msto looks good to me based on your comments!

pybedlite/bed_record.py Show resolved Hide resolved
pybedlite/bed_record.py Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
@msto
Copy link
Contributor Author

msto commented Mar 12, 2024

@nh13 Minor changes to address the circular import and standardize on from_FOO() constructors on both classes.

lmk if it still lgty

@msto msto requested a review from nh13 March 12, 2024 13:19
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.

Can you just try the one suggestion, and if that doesn't work, ship it?

pybedlite/bed_record.py Show resolved Hide resolved
pybedlite/bed_record.py Show resolved Hide resolved
@msto
Copy link
Contributor Author

msto commented Mar 20, 2024

for posterity

without importing Interval, both flake8 and mypy complain about the forward reference

pybedlite/bed_record.py:196:57: F821 undefined name 'Interval'
    def from_interval(cls: Type["BedRecord"], interval: "Interval") -> "BedRecord":
pybedlite/bed_record.py:196: error: Name "Interval" is not defined  [name-defined]

@msto msto merged commit 674567c into main Mar 20, 2024
6 checks passed
@msto msto deleted the ms_alternate-bed-constructors branch March 20, 2024 18:30
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