-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
SeqIO parse side effect remove #4280
base: master
Are you sure you want to change the base?
Conversation
@BabaYaga1221 Thank you. Some of the tests are failing, can you have a look? |
Okay sir, I will try handle them, but can provide assistance about where and why the test fails.@mdehoon |
Test failures on Linux from the CI:
It appears to be closing the file earlier than those tests expect. You should be able to run those test files alone to reproduce this, e.g. |
I realized that our design of For comparison, |
Not sure what's going on with the CI dependencies, but yes this is unrelated:
|
Change 2f188af might help, that file was requesting quite an old version of black. Separately we should avoid that inconsistency in future if possible... |
See #4281 for the codecov problem (it was deprecated and has been removed from PyPI). |
cbc9ee8
to
f91a967
Compare
Please revert your change for #820, this is not a self-test but deliberate functionality when running the file directly. See #820 (comment) |
I apologize for my confusion earlier. I understand now that I accidentally committed code to the same branch that I was using to solve a separate issue related to SeqIO. I am reverting the changes. |
f91a967
to
3ce8e66
Compare
@peterjc Sir, Can you help me that what should I need to change for resolving this issue. |
4410f6e
to
1d3d05e
Compare
@peterjc As suggested, I implemented the changes and the total count failures in appveyor decreased from 57 to 16, still some test fails. |
Lots of errors |
This is looking much more promising... CircleCI has failed while trying to install pycairo, so unrelated to your changes. |
@peterjc I appreciate your help, Sir, but I've been struggling with this code for 15 days. Could you please review it and let me know if any changes are suggested? |
@BabaYaga1221 the only stylistic change I have now is to remove rather than just comment out the old lines @mdehoon over to you to review the overall changes (the history is messy, I'd squash-and-merge this if the changes are OK), since you are much more familiar with writing context managers than I am. |
@BabaYaga1221 Please wait a week before asking again. |
Bio/SeqIO/Interfaces.py
Outdated
raise | ||
|
||
def __next__(self): | ||
"""Return the next entry.""" | ||
try: | ||
return next(self.records) | ||
except Exception: | ||
if self.should_close_stream: | ||
if self.stream is not self.source: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to close the stream here.
Bio/SeqIO/Interfaces.py
Outdated
raise | ||
|
||
def __next__(self): | ||
"""Return the next entry.""" | ||
try: | ||
return next(self.records) | ||
except Exception: | ||
if self.should_close_stream: | ||
self.stream.close() | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the try:
except:
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if the record is empty then, it might generate error and that's why I add try:
except:
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, end of file and no more records does seem likely to trigger this try/except.
Bio/SeqIO/Interfaces.py
Outdated
try: | ||
self.records = self.parse(self.stream) | ||
except Exception: | ||
if self.should_close_stream: | ||
self.stream.close() | ||
self.stream.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not need to close the stream here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the as per your suggestion.
Did you run the tests locally? They're failing now:
|
actually, I didn't run them locally, And -
These errors might be due to the strange design of |
Well, I didn't add a beginners tag to #4252 deliberately. The changes looked reasonable (prior to the last commit which broke the tests), but may still be doing something change. If you haven't already tried it, my advice is try adding logging to all the key methods (at least init, enter, exit, next) to examine what is happening with some simple use cases. |
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 runpre-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
andCONTRIB.rst
as part of this pull request, am listedalready, or do not wish to be listed. (This acknowledgement is optional.)
Closes #...
Add
__entry__
and__exit__
inBio/SeqIO/Interfaces.py
.@mdehoon @peterjc