Add type annotations by szabgab · Pull Request #5087 · biopython/biopython

Add type annotations by szabgab · Pull Request #5087 · biopython/biopython

[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.rst file, have run pre-commit
locally, 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.rst and CONTRIB.rst as part of this pull request, am listed
already, or do not wish to be listed. (This acknowledgement is optional.)

Addresses #2236 and does not touch files that are addressed in #5057

		"""
		try:
		table_id = int(table)
		# TODO: only call int if table is of type int

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try/except is the check for it being an int, or int like.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to change this to isinstance(table, int) and remove the try/except ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be something else int-like but not a subclass. I would be cautious about changing this for fear of unintended side effects.



		def reverse_complement(sequence, inplace=False):
		def reverse_complement(sequence: Seq, inplace: bool = False) -> str:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?



		def reverse_complement_rna(sequence, inplace=False):
		def reverse_complement_rna(sequence: Seq, inplace: bool = False) -> str:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?



		def complement(sequence, inplace=False):
		def complement(sequence: Seq, inplace: bool = False) -> str:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?



		def complement_rna(sequence, inplace=False):
		def complement_rna(sequence: Seq, inplace: bool = False) -> str:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely Union[str, Seq, MutableSeq, SeqRecord] as per the docstring?

		def translate(
		sequence, table="Standard", stop_symbol="*", to_stop=False, cds=False, gap=None
		):
		sequence: Seq,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely Union[str, Seq, MutableSeq] as per the docstring?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots to fix here - see comments.

		def _translate_str(
		sequence, table, stop_symbol="*", to_stop=False, cds=False, pos_stop="X", gap=None
		):
		sequence: Seq,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).