Re-organizing key dataclasses into a single model.py and then exposing those for import from prymer.
#109
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #109 +/- ##
==========================================
- Coverage 97.37% 97.32% -0.06%
==========================================
Files 23 21 -2
Lines 1677 1642 -35
Branches 198 195 -3
==========================================
- Hits 1633 1598 -35
Misses 26 26
Partials 18 18 ☔ View full report in Codecov by Sentry. |
WalkthroughThe pull request introduces a significant architectural refactoring of the The changes involve removing several API modules like Additionally, the project's Python version requirement has been upgraded from 3.11 to 3.12, and the version number has been incremented to 3.0.0, signaling a major architectural revision. A new dependency on Finishing Touches
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: 4
🧹 Nitpick comments (7)
tests/conftest.py (1)
31-38: Add docstring and consider session scope.Fixture needs documentation. Consider session scope if dictionary is immutable.
@pytest.fixture +@pytest.fixture(scope="session") def seq_dict() -> SequenceDictionary: + """Returns a SequenceDictionary with three chromosomes for testing.""" metadatas: list[SequenceMetadata] = [prymer/model.py (2)
27-30: Clarify inequality conditions in docstringIn the
MinOptMaxclass docstring, the constraints can be more precisely stated. Consider rephrasing as:"
minmust be less than or equal toopt, andoptmust be less than or equal tomax."This clarifies the allowable equality in the conditions.
707-707: Specify strand when creating ampliconSpanIn
calculate_amplicon_span, the strand defaults toStrand.POSITIVE. If strand orientation is significant for the amplicon, explicitly set the strand based on primer strands.prymer/api/coordmath.py (1)
1-2: Update module docstring.Current docstring is too vague after removal of methods.
-# Methods for coordinate-based math. +"""Methods for calculating coordinate-based intervals and positions."""tests/primer3/test_primer3_input_tag.py (1)
10-11: Document Python version requirement.Convert comment to docstring to better document the version constraint.
- # This is supported in python 3.12+, but not in 3.11 - # assert f"{tag}" in Primer3InputTag + """ + Note: The following assertion is only supported in Python 3.12+: + assert f"{tag}" in Primer3InputTag + """prymer/primer3/primer3_input_tag.py (1)
32-33: Move developer note to class docstring.Important implementation detail should be in class docstring for better visibility.
class Primer3InputTag(UppercaseStrEnum): """ Enumeration of Primer3 input tags. Please see the Primer3 manual for additional details: https://primer3.org/manual.html#commandLineTags This class represents two categories of Primer3 input tags: * `SEQUENCE_` tags are those that control sequence-specific attributes of Primer3 jobs. These can be modified after each query submitted to Primer3. * `PRIMER_` tags are those that describe more general parameters of Primer3 jobs. These attributes persist across queries to Primer3 unless they are explicitly reset. Errors in these "global" input tags are fatal. + + Important: sequence-specific tags must be specified prior to global tags """ - # Developer note: sequence-specific tags must be specified prior to global tagsprymer/primer3/primer3.py (1)
247-248: Remove unnecessarysuper().__exit__call
AbstractContextManager.__exit__does nothing. Removing this call simplifies the code.Apply this diff:
- super().__exit__(exc_type, exc_value, traceback)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.github/workflows/tests.yml(2 hunks).github/workflows/wheels.yml(1 hunks)prymer/__init__.py(1 hunks)prymer/api/__init__.py(0 hunks)prymer/api/clustering.py(0 hunks)prymer/api/coordmath.py(1 hunks)prymer/api/minoptmax.py(0 hunks)prymer/api/oligo.py(0 hunks)prymer/api/oligo_like.py(0 hunks)prymer/api/picking.py(2 hunks)prymer/api/primer.py(0 hunks)prymer/api/primer_pair.py(0 hunks)prymer/api/span.py(0 hunks)prymer/api/variant_lookup.py(1 hunks)prymer/model.py(1 hunks)prymer/offtarget/offtarget_detector.py(2 hunks)prymer/primer3/primer3.py(14 hunks)prymer/primer3/primer3_input.py(2 hunks)prymer/primer3/primer3_input_tag.py(1 hunks)prymer/primer3/primer3_parameters.py(1 hunks)prymer/primer3/primer3_task.py(2 hunks)pyproject.toml(4 hunks)tests/api/conftest.py(0 hunks)tests/api/test_clustering.py(0 hunks)tests/api/test_coordmath.py(1 hunks)tests/api/test_oligo_like.py(0 hunks)tests/api/test_picking.py(1 hunks)tests/api/test_variant_lookup.py(1 hunks)tests/conftest.py(2 hunks)tests/model/test_minoptmax.py(1 hunks)tests/model/test_oligo.py(9 hunks)tests/model/test_primer_pair.py(2 hunks)tests/model/test_span.py(1 hunks)tests/offtarget/test_offtarget.py(1 hunks)tests/primer3/test_primer3.py(7 hunks)tests/primer3/test_primer3_input.py(1 hunks)tests/primer3/test_primer3_input_tag.py(1 hunks)tests/primer3/test_primer3_parameters.py(1 hunks)tests/primer3/test_primer3_task.py(1 hunks)
💤 Files with no reviewable changes (11)
- tests/api/conftest.py
- prymer/api/primer.py
- prymer/api/primer_pair.py
- prymer/api/init.py
- prymer/api/oligo.py
- tests/api/test_oligo_like.py
- prymer/api/clustering.py
- prymer/api/minoptmax.py
- prymer/api/oligo_like.py
- tests/api/test_clustering.py
- prymer/api/span.py
✅ Files skipped from review due to trivial changes (12)
- tests/model/test_minoptmax.py
- prymer/primer3/primer3_input.py
- tests/primer3/test_primer3_task.py
- prymer/api/variant_lookup.py
- tests/api/test_variant_lookup.py
- tests/primer3/test_primer3_parameters.py
- prymer/primer3/primer3_task.py
- tests/offtarget/test_offtarget.py
- tests/api/test_picking.py
- prymer/primer3/primer3_parameters.py
- prymer/api/picking.py
- prymer/offtarget/offtarget_detector.py
🔇 Additional comments (24)
tests/conftest.py (1)
8-12: Import changes look good.Imports align with the PR objective of consolidating dataclasses into model.py.
prymer/model.py (2)
298-304: Handle missing reference names in_to_tupleIn the
Spanmethod_to_tuple, accessingseq_dict.by_name(self.refname).indexmay raise aKeyErrorifself.refnameis not inseq_dict. Validate thatself.refnameexists before accessing to prevent unexpected errors.
569-573: Catch exceptions during amplicon computationIn
PrimerPair.__post_init__, callingself.ampliconmay raise aValueErrorif primers are misconfigured. Consider handling exceptions to provide clearer messages or validate inputs before computing the amplicon.prymer/__init__.py (1)
1-7: Exports enhanced for user convenienceRe-exporting
MinOptMax,Oligo,PrimerPair,Span, andStranddirectly fromprymersimplifies imports for users, aligning with the PR objectives.tests/api/test_coordmath.py (1)
1-7: Ensure complete test coverage after removalsWith the removal of tests for
get_locus_stringandrequire_same_refname, verify that all remaining functions incoordmathare adequately tested. Consider adding tests for any untested functions.prymer/api/coordmath.py (1)
Line range hint
1-24: Verify removed methods.Confirm that
require_same_refname()andget_locus_string()were moved tomodel.py.tests/primer3/test_primer3_input.py (1)
3-5: LGTM!Import paths correctly updated to use the new model module.
tests/model/test_span.py (2)
Line range hint
1-300: LGTM!Test coverage remains comprehensive after restructuring. All key Span functionality is well-tested.
4-5: Verify BedLikeCoords removal impact.Confirm that BedLikeCoords functionality is either deprecated or moved to model.py.
✅ Verification successful
BedLikeCoords functionality consolidated into Span class.
Span class now handles all coordinate operations with improved functionality. Removal is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining BedLikeCoords references rg "BedLikeCoords"Length of output: 18
Script:
#!/bin/bash # Check git history for BedLikeCoords git log -S "BedLikeCoords" --oneline # Examine current model.py content cat prymer/model.pyLength of output: 28053
prymer/primer3/primer3_input_tag.py (2)
34-37: LGTM!Clean implementation with clear docstring.
39-42: LGTM!Logical implementation leveraging is_sequence_arg.
tests/model/test_oligo.py (2)
9-11: LGTM!Import changes align with module reorganization.
196-196: LGTM!Property access is more Pythonic than method calls.
Also applies to: 248-248
tests/model/test_primer_pair.py (1)
9-12: LGTM!Import changes align with module reorganization.
tests/primer3/test_primer3.py (4)
10-14: LGTM!Import changes align with module reorganization.
162-162: LGTM!More specific error message improves debugging.
190-190: LGTM!Property access is more Pythonic than method calls.
Also applies to: 237-237, 448-448, 452-452, 469-469
364-364: LGTM!Error message better reflects the I/O nature of the failure.
.github/workflows/wheels.yml (1)
14-15: Clarify Python version support strategy.Dropping Python 3.11 support needs explanation. If temporary, add timeline for re-enabling.
.github/workflows/tests.yml (2)
19-20: Match Python version strategy with wheels.yml.Version matrix changes should be synchronized with wheels.yml.
60-63: Add timeline for re-enabling documentation build.Disabled documentation builds risk documentation drift. Set deadline for re-enabling.
pyproject.toml (2)
121-122: Confirm primer3 type hints.Added primer3 to mypy ignores. Check if type hints are available.
✅ Verification successful
External primer3-py package type hints can be safely ignored.
Local primer3 implementation has complete type hints. Mypy ignore is for external dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for primer3 type hints pip show primer3-py | grep -i "typing"Length of output: 132
Script:
#!/bin/bash # Search for primer3 type hint files and usage echo "=== Searching for .pyi files ===" fd -e pyi primer3 echo "=== Checking for type hints usage ===" rg "from typing import|import typing" -A 2 | grep -i primer3 echo "=== Looking for primer3 imports with type annotations ===" rg "primer3.*:.*" -A 2Length of output: 8714
35-35: Verify dependency compatibility.New Python 3.12 requirement and primer3-py addition need compatibility verification.
Also applies to: 42-42
✅ Verification successful
Python 3.12 and primer3-py are compatible.
primer3-py 2.0.3 has official Python 3.12 wheels.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check dependency compatibility poetry check poetry lock --checkLength of output: 230
Script:
#!/bin/bash # Find requirements files and check their content fd requirements.txt --exec cat {} # Find primer3-py imports and usage rg "primer3" -l # Check setup.py if exists fd setup.py --exec cat {} # Look for primer3 in install_requires rg "install_requires" -A 5Length of output: 900
Script:
#!/bin/bash # Check poetry.lock for primer3-py version cat poetry.lock | grep -A 10 "primer3-py" # Check installation docs cat docs/installation-and-developers-documentation.md # Check pyproject.toml dependencies section cat pyproject.toml | grep -A 10 "\[tool.poetry.dependencies\]"Length of output: 5466
prymer/primer3/primer3.py (1)
123-123: Ensure 'Self' type is compatibleThe
Selftype hint is available in Python 3.11 and above. If earlier versions need support, replaceSelfwith the class name in quotes.
pyproject.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| [tool.poetry] | |||
| name = "prymer" | |||
| version = "2.2.0" | |||
| version = "3.0.0" | |||
There was a problem hiding this comment.
💡 Codebase verification
Add CHANGELOG.md to document breaking changes
Version 3.0.0 indicates breaking changes. Create CHANGELOG.md in repository root to document them.
🔗 Analysis chain
Document breaking changes for version 3.0.0.
Major version bump requires changelog entry detailing breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CHANGELOG updates
rg -i "version.*3\.0\.0|breaking.*changes" CHANGELOG.md || echo "No changelog entry found for 3.0.0"
Length of output: 189
Script:
#!/bin/bash
# Search for changelog files in various formats
fd -i 'changelog|changes|history|news' --type f
Length of output: 49
prymer/primer3/primer3.py
Outdated
| def __enter__(self) -> Self: | ||
| return self |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust return type in __enter__ method
Replace -> Self with -> 'Primer3' for compatibility with Python versions before 3.11.
Apply this diff:
- def __enter__(self) -> Self:
+ def __enter__(self) -> 'Primer3':📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __enter__(self) -> Self: | |
| return self | |
| def __enter__(self) -> 'Primer3': | |
| return self |
prymer/primer3/primer3.py
Outdated
| max_dinuc_bases: int = -1 | ||
| if design_input.task.requires_primer_amplicon_params: | ||
| max_dinuc_bases = design_input.primer_and_amplicon_params.primer_max_dinuc_bases | ||
| elif design_input.task.requires_probe_params: | ||
| max_dinuc_bases = design_input.probe_params.probe_max_dinuc_bases | ||
|
|
||
| return oligo_design.longest_dinucleotide_run_length() <= max_dinuc_bases | ||
| return oligo_design.longest_dinucleotide_run_length <= max_dinuc_bases |
There was a problem hiding this comment.
Handle uninitialized max_dinuc_bases
If neither condition is met, max_dinuc_bases remains -1, leading to incorrect comparisons. Add an else clause to raise an error.
Apply this diff:
elif design_input.task.requires_probe_params:
max_dinuc_bases = design_input.probe_params.probe_max_dinuc_bases
+ else:
+ raise ValueError("Design task must require primer or probe parameters.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| max_dinuc_bases: int = -1 | |
| if design_input.task.requires_primer_amplicon_params: | |
| max_dinuc_bases = design_input.primer_and_amplicon_params.primer_max_dinuc_bases | |
| elif design_input.task.requires_probe_params: | |
| max_dinuc_bases = design_input.probe_params.probe_max_dinuc_bases | |
| return oligo_design.longest_dinucleotide_run_length() <= max_dinuc_bases | |
| return oligo_design.longest_dinucleotide_run_length <= max_dinuc_bases | |
| max_dinuc_bases: int = -1 | |
| if design_input.task.requires_primer_amplicon_params: | |
| max_dinuc_bases = design_input.primer_and_amplicon_params.primer_max_dinuc_bases | |
| elif design_input.task.requires_probe_params: | |
| max_dinuc_bases = design_input.probe_params.probe_max_dinuc_bases | |
| else: | |
| raise ValueError("Design task must require primer or probe parameters.") | |
| return oligo_design.longest_dinucleotide_run_length <= max_dinuc_bases |
prymer/primer3/primer3.py
Outdated
| @dataclass(init=True, slots=True, frozen=True) | ||
| class Primer3Result(Generic[OligoLikeType]): | ||
| class Primer3Result[ResultType: (Oligo, PrimerPair)]: |
There was a problem hiding this comment.
Fix invalid generic class syntax
The declaration class Primer3Result[ResultType: (Oligo, PrimerPair)]: is invalid. Use TypeVar and Generic from the typing module to define a generic class correctly.
Apply this diff:
+ from typing import TypeVar, Generic
+ ResultType = TypeVar('ResultType', Oligo, PrimerPair)
- class Primer3Result[ResultType: (Oligo, PrimerPair)]:
+ class Primer3Result(Generic[ResultType]):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Primer3Result[ResultType: (Oligo, PrimerPair)]: | |
| from typing import TypeVar, Generic | |
| ResultType = TypeVar('ResultType', Oligo, PrimerPair) | |
| class Primer3Result(Generic[ResultType]): |
| - [`get_closed_end()`][prymer.api.coordmath.get_closed_end] -- gets the closed end of an | ||
| interval given its start and length. |
| from prymer.model import MinOptMax | ||
| from prymer.model import Oligo | ||
| from prymer.model import PrimerPair | ||
| from prymer.model import Span |
There was a problem hiding this comment.
Shall we prefer importing from the top-level, here and elsewhere? Or is there a reason for the preference to import it from model?
| from prymer.model import MinOptMax | |
| from prymer.model import Oligo | |
| from prymer.model import PrimerPair | |
| from prymer.model import Span | |
| from prymer import MinOptMax | |
| from prymer import Oligo | |
| from prymer import PrimerPair | |
| from prymer import Span |
| @@ -11,7 +11,7 @@ | |||
|
|
|||
| ```python | |||
| >>> from pathlib import Path | |||
| >>> from prymer.api.span import Strand | |||
| >>> from prymer.model import Strand | |||
There was a problem hiding this comment.
I think in the docs we should import these as we would recommend users to import them, namely from the top-level prymer namespace. If you agree, can you fix in the rest of the modules as well?
| >>> from prymer.model import Strand | |
| >>> from prymer import Strand |
|
Answering here @nh13 rather than to individual comments. On the imports - it is unfortunately necessary that within the That said, all examples and doc-tests can be updated to use the re-exported location(s) and I've done that. Additionally tests can import from the top level too, and I have also updated those. |
Instead of having a module per class, this consolidates several classes including Strand, Span, Oligo, PrimerPair etc. into a single module. All the symbols are then re-exported so that they can be accessed via: