-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use Sequence instead of list[] in parameters to build_primer_pairs() #86
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
=======================================
Coverage 96.63% 96.64%
=======================================
Files 26 26
Lines 1696 1697 +1
Branches 330 330
=======================================
+ Hits 1639 1640 +1
Misses 31 31
Partials 26 26 ☔ View full report in Codecov by Sentry. |
WalkthroughThe pull request introduces changes to two primary components: the Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.gitignore
(1 hunks)prymer/api/picking.py
(2 hunks)
🔇 Additional comments (2)
prymer/api/picking.py (2)
19-19
: LGTM! Good choice of using Sequence
type.
Using Sequence
from collections.abc
is more appropriate than Iterable
as it provides index access which is needed for accessing the primers in the function.
94-95
: LGTM! Parameter type change improves flexibility while maintaining compatibility.
The change from list[Oligo]
to Sequence[Oligo]
successfully:
- Allows subclasses of
Oligo
to be used (e.g., for storing QC attributes) - Maintains backward compatibility as
list
implementsSequence
- Preserves all existing functionality as the function only needs sequence operations
Let's verify that the function only uses sequence operations:
✅ Verification successful
Parameter type change to Sequence[Oligo]
is safe and appropriate
After analyzing the implementation, the function only uses sequence operations that are compatible with the Sequence
protocol:
- Basic iteration using
for
loops - Boolean check with
any()
- No list-specific methods or operations are used
The change improves API flexibility while maintaining full compatibility with existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the function only uses operations compatible with Sequence
# Expected: Only sequence operations (iteration, len, indexing) should be found
# Search for method calls on left_primers and right_primers
rg -U --multiline '(left|right)_primers\.[a-zA-Z_]+\(' prymer/api/picking.py
# Search for any special list operations
rg -U --multiline '(left|right)_primers\[(.*?)\]' prymer/api/picking.py
Length of output: 152
Script:
#!/bin/bash
# Let's try a different approach to verify sequence operations
# Get the entire function implementation to analyze usage
# First, find the function name that contains these parameters
rg -l "left_primers.*right_primers.*Sequence\[Oligo\]" prymer/api/picking.py
# Then get the full function implementation with context
rg -U --multiline "def.*left_primers.*right_primers.*Sequence\[Oligo\]" -A 50 prymer/api/picking.py
Length of output: 180
Script:
#!/bin/bash
# Let's try a simpler approach to get the function content
# First, get the file content to see the actual implementation
cat prymer/api/picking.py
Length of output: 7063
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason not to implement coderabbit's suggestion here, I never would have caught that
I went the other way - the JetBrains section at the bottom was commented out and not that useful, and I think it's nice to have the ignores for all the IDEs in the same place. |
Makes sense to me. I was more commenting that I never would have noticed the duplication - I'm pretty lenient about additions to the gitignore 😅 |
Fixes #84