refactor: Make PrimerPair.amplicon a cached property
#93
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #93 +/- ##
==========================================
- Coverage 96.67% 96.66% -0.01%
==========================================
Files 26 26
Lines 1712 1708 -4
Branches 333 333
==========================================
- Hits 1655 1651 -4
Misses 31 31
Partials 26 26 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
WalkthroughThe pull request modifies the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)prymer/api/primer_pair.py (3)
Import changes align with the refactoring goal.
Cached property implementation maintains validation while improving efficiency.
Adding ✅ Verification successfulNo impact from kw_only change All existing PrimerPair instantiations use keyword arguments. Change is safe. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for PrimerPair instantiations that might use positional args
rg "PrimerPair\([^,]*,[^,]*[^)]" --type py
Length of output: 193 Script: #!/bin/bash
# Check all PrimerPair instantiations in tests and examples
rg "PrimerPair\(" --type py -A 2
Length of output: 2055 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prymer/api/primer_pair.py (1)
83-86: Good use of cached_property for performance.Caches expensive calculation while maintaining clean API. Consider memory impact if creating many instances.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
prymer/api/primer_pair.py(3 hunks)pyproject.toml(1 hunks)
🔇 Additional comments (4)
pyproject.toml (2)
Line range hint 33-33: Verify pyproject_hooks version constraint
Version constraint !=1.1.0 suggests known issue.
#!/bin/bash
# Check for issues with pyproject_hooks 1.1.0
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "pyproject_hooks") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
}
}
}'118-118: Verify excluded directories exist
Ensure site/ and docs/ directories exist and are in correct locations.
✅ Verification successful
Exclude configuration is valid
docs/ exists and site/ is a standard MkDocs build output directory (confirmed by mkdocs.yml presence). Exclusions are appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if excluded directories exist
fd -t d "^(site|docs)$"
Length of output: 293
Script:
#!/bin/bash
# Check for docs directory in a more thorough way
fd -t d -d 1 "^docs$"
# Check for site directory and any potential mkdocs config
fd -t d -d 2 "site$"
fd "mkdocs.ya?ml$"
Length of output: 84
prymer/api/primer_pair.py (2)
46-46: LGTM: Import placement is correct.
58-58: Verify no initialization issues with frozen dataclass.
Class definition looks good. Let's verify no initialization issues.
✅ Verification successful
No initialization issues found with frozen dataclass
_ampliconfield was safely removed, replaced with@property amplicon- No
__post_init__usage found - All
ampliconreferences properly use the property accessor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any __post_init__ or _amplicon references
rg -l "_amplicon|__post_init__"
Length of output: 609
Script:
#!/bin/bash
# Check for _amplicon usage in primer_pair.py and its tests
rg "_amplicon" prymer/api/primer_pair.py tests/api/test_primer_pair.py -C 2
# Check for __post_init__ in primer_pair.py and tests
rg "__post_init__" prymer/api/primer_pair.py tests/api/test_primer_pair.py -C 2
Length of output: 6110
|
|
||
| def __post_init__(self) -> None: |
There was a problem hiding this comment.
The problem here is that the validation that the LP/RP are oriented sensibly with respect to each other now won't run until/unless a user accesses the amplicon span.
There was a problem hiding this comment.
I don’t think that’s true, as evidenced by these tests continuing to pass.
prymer/tests/api/test_primer_pair.py
Lines 422 to 457 in ebc72f7
Post-init checks perform the validation at the time of instantation. In this case, the validation is in the parent class’s post-init. The post-init for OligoLike references the instance’s span, which just returns the value of amplicon (I’ve previously suggested removing such synonymous properties so this kind of logic is less obfuscated) and therefore calls the validation code in question.
prymer/prymer/api/oligo_like.py
Line 59 in ebc72f7
I think this is bad design for a number of reasons. One, abstract base classes should be abstract. They should declare an interface but not provide implementation. Two, this is an example of DRY going bad in practice - the logic for validating a primer pair is different from validating a single oligo, but we’ve forced both to use the same code. Three, hiding the logic away from the dataclass where it’s used will naturally lead to the kind of confusion you just ran into.
I would also like to refactor to fix that, but in the interest of not pulling threads upon threads, I kept this PR more tightly scoped.
There was a problem hiding this comment.
Ugh, ok. So a potential foot-gun to be dealt with as soon as we deal with PrimerLike.
msto
force-pushed
the
ms_amplicon-attr
branch
from
8318695
to
6fcd304
Compare
See this discussion for context: #89 (comment)
This PR also updates the mypy config to exclude the
mkdocsdirectories.