Skip to content
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.

Sign up for GitHub

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

Jump to bottom

File.as_handle_and_flag #4410

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

File.as_handle_and_flag #4410

wants to merge 1 commit into from

Conversation

speleo3
Copy link
Contributor

@speleo3 speleo3 commented Aug 11, 2023

Follow-up on #4377 ("mypy pre-commit hook").

  • New function as_handle_and_flag
  • New function check_handle_mode
  • Use as_handle or as_handle_and_flag where appropriate
  • Move _PathLikeTypes, _IOSource and _TextIOSource to Bio.File module

  • I hereby agree to dual licence this and any previous contributions under both
    the Biopython License Agreement AND the BSD 3-Clause License.

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

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

@speleo3 speleo3 mentioned this pull request Aug 11, 2023
Merged
3 tasks
speleo3
speleo3 commented Aug 12, 2023
View reviewed changes
Bio/File.py Show resolved Hide resolved
peterjc
peterjc reviewed Aug 12, 2023
View reviewed changes
Bio/SeqIO/QualityIO.py Outdated Show resolved Hide resolved
@peterjc
Copy link
Member

peterjc commented Aug 12, 2023

I like the more consistent use of as_handle here

@mdehoon
Copy link
Contributor

mdehoon commented Nov 19, 2023

I am not a big fan of utility functions, as it is one more API to remember. In the past, we spent some effort removing such utility functions from Biopython because they were not consistently being used by developers, as they were not always aware of their existence. So I am hesitant to add more utility functions.
Notice also that the yield statement in as_handle creates an iterator class behind the scenes, and the simple operation of opening a file becomes an iteration.
In Bio.SeqIO, the iterator design is a bit strange in places. For example, FastaIterator inherits from SequenceIterator; calling next on FastaIterator calls next on self.records, which points to the return value of self.iterate, which is a generator as it uses yield. So now we have a double iterator.

@peterjc
Copy link
Member

peterjc commented Nov 19, 2023

On the other hand, the helper function avoids a lot of repetitive boiler plate code. So I like this change in general.

I'm not sure we need the extra function as_handle_and_flag though - as it stands it only gets used once?

Any accidental double iteration in FastaIterator is best flagged separately - it sounds like the SequenceIterator isn't quite right.

@mdehoon
Copy link
Contributor

mdehoon commented Nov 19, 2023

I think the first step would be to fix the SequenceIterator. Once that is done, it will be more clear what a utility function should actually look like. Probably it should not be a function, but a class from which the SequenceIterator inherits.

The challenge is that SequenceIterator and its equivalent in Bio.Align etc. are non-trivial. I don't mind a bit of code duplication if it makes the class easier to understand, rather than adding a utility function as one more component into the mix.

@speleo3
Copy link
Contributor Author

speleo3 commented Nov 19, 2023

Opening a file with as_handle does not really become an iterator. It becomes a context manager via the contextlib.contextmanager decorator.

as_handle_and_flag is used 4 times.

@mdehoon mdehoon mentioned this pull request Aug 25, 2024
Open
@speleo3
File.as_handle_and_flag
57e8b9c 
- New function as_handle_and_flag
- New function check_handle_mode
- Use as_handle or as_handle_and_flag where appropriate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterjc peterjc peterjc left review comments

@mdehoon mdehoon Awaiting requested review from mdehoon mdehoon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@speleo3 @peterjc @mdehoon