Reference set builder dev

[sam-white04]

Looking for feedback on fgpyo/fasta/reference_set_builder.py ~
Two classes ReferenceSetBuilder and ReferenceBuilder, attempts to follow the scala version as much as possible. Still need to add .fai and .dict.

[codecov-commenter]

Codecov Report

Base: 92.10% // Head: 92.11% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (d31a292) compared to base (e5689ee).
Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #24   +/-   ##
=======================================
  Coverage   92.10%   92.11%           
=======================================
  Files          20       22    +2     
  Lines        2243     2334   +91     
  Branches      402      414   +12     
=======================================
+ Hits         2066     2150   +84     
- Misses        130      136    +6     
- Partials       47       48    +1     

Impacted Files	Coverage Δ
fgpyo/fasta/builder.py	87.50% <87.50%> (ø)
fgpyo/fasta/tests/test_builder.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tfenne]

Don't worry about the volume of comments - this is fairly normal for our PR reviews!

One thought about how the code is laid out in files... Notice how the SamBuilder is in the __init__.py for the sam module so it imports as from fgpyo.sam import SamBuilder. Your current choice makes it from fgpyo.fasta.reference_set_builder import ReferenceSetBuilder which is both long and redundant. I would suggest i) putting into the __init.py__ for the fasta module and ii) renaming it to FastaBuilder which is shorter and more obvious (not sure why we didn't name it that in scala?)

[sam-white04]

I have addressed most of @nh13 and @tfenne comments. This review is a house-keeping document to keep track of the changes I have made in response to feedback.

[nh13]

One minor comment, and agree with @tfenne's suggestions, so I'll approve assuming you'll fix those.

[sam-white04]

@tfenne Thank you for the feedback, I have addressed all your comments on fasta/builder.py. Most notably removing the default for attributes species/assembly in ContigBuilder initialization, and amending parameters species/assembly in FastaBuilder.add() to be optional with the default FastaBuilder.species/assembly.

[sam-white04]

@tfenne Thank you for the feedback, I have addressed all your comments on fasta/builder.py. Most notably removing the default for attributes species/assembly in ContigBuilder initialization, and amending parameters species/assembly in FastaBuilder.add() to be optional with the default FastaBuilder.species/assembly.