Conversation
| """ | ||
| try: | ||
| table_id = int(table) | ||
| # TODO: only call int if table is of type int |
There was a problem hiding this comment.
This try/except is the check for it being an int, or int like.
There was a problem hiding this comment.
Wouldn't it be better to change this to isinstance(table, int) and remove the try/except ?
There was a problem hiding this comment.
It could be something else int-like but not a subclass. I would be cautious about changing this for fear of unintended side effects.
Bio/Seq.py
Outdated
|
|
||
|
|
||
| def reverse_complement(sequence, inplace=False): | ||
| def reverse_complement(sequence: Seq, inplace: bool = False) -> str: |
There was a problem hiding this comment.
Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?
Bio/Seq.py
Outdated
|
|
||
|
|
||
| def reverse_complement_rna(sequence, inplace=False): | ||
| def reverse_complement_rna(sequence: Seq, inplace: bool = False) -> str: |
There was a problem hiding this comment.
Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?
Bio/Seq.py
Outdated
|
|
||
|
|
||
| def complement(sequence, inplace=False): | ||
| def complement(sequence: Seq, inplace: bool = False) -> str: |
There was a problem hiding this comment.
Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?
Bio/Seq.py
Outdated
|
|
||
|
|
||
| def complement_rna(sequence, inplace=False): | ||
| def complement_rna(sequence: Seq, inplace: bool = False) -> str: |
There was a problem hiding this comment.
Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?
Bio/Seq.py
Outdated
| def translate( | ||
| sequence, table="Standard", stop_symbol="*", to_stop=False, cds=False, gap=None | ||
| ): | ||
| sequence: Seq, |
There was a problem hiding this comment.
Surely Union[str, Seq, MutableSeq] as per the docstring?
peterjc
left a comment
peterjc
left a comment
There was a problem hiding this comment.
Lots to fix here - see comments.
Bio/Seq.py
Outdated
| def _translate_str( | ||
| sequence, table, stop_symbol="*", to_stop=False, cds=False, pos_stop="X", gap=None | ||
| ): | ||
| sequence: Seq, |
There was a problem hiding this comment.
No, as per the docstring, this should be a string.
* In some cases a new, temporary variable called `encoded_sequence` was used as that value has a different type. * In 3 places attr-defined had to be ignored as mypy complained about missing methods in SeqRecord. This probably needs to be addressed.
|
Thanks for the feedback. I hope this one is going to be a step forward. |
| from Bio import BiopythonWarning | ||
| from Bio.Data import CodonTable | ||
| from Bio.Data import IUPACData | ||
| from Bio.SeqRecord import SeqRecord |
There was a problem hiding this comment.
You'll have to remove that, or put it an if-typing conditional or something. It is making a circular import (see the CI failures).
3 participants
[X ] I hereby agree to dual licence this and any previous contributions under both
the Biopython License Agreement AND the BSD 3-Clause License.
[ X] I have read the
CONTRIBUTING.rstfile, have runpre-commitlocally, and understand that continuous integration checks will be used to
confirm the Biopython unit tests and style checks pass with these changes.
[ X] I have added my name to the alphabetical contributors listings in the files
NEWS.rstandCONTRIB.rstas part of this pull request, am listedalready, or do not wish to be listed. (This acknowledgement is optional.)
Addresses #2236 and does not touch files that are addressed in #5057