-
Notifications
You must be signed in to change notification settings - Fork 663
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
Remove py2 things from lib #2752
Conversation
package/MDAnalysis/lib/util.py
Outdated
@@ -438,7 +435,7 @@ def _get_stream(filename, openfunction=open, mode='r'): | |||
# case we have to ignore the error and return None. Second is when openfunction can't open the file because | |||
# either the file isn't there or the permissions don't allow access. | |||
if errno.errorcode[err.errno] in ['ENOENT', 'EACCES']: | |||
six.reraise(*sys.exc_info()) | |||
raise sys.exc_info()[0](sys.exc_info()[1]) from err |
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'm not sure this is the right conversion here, any input would be appreciated
Codecov Report
@@ Coverage Diff @@
## develop #2752 +/- ##
===========================================
- Coverage 91.25% 91.07% -0.18%
===========================================
Files 176 177 +1
Lines 23967 23655 -312
Branches 3153 3122 -31
===========================================
- Hits 21871 21544 -327
- Misses 1470 1490 +20
+ Partials 626 621 -5
Continue to review full report at Codecov.
|
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.
LGTM, I agree that raising from None is probably the desired behaviour and that you probably don't need all the exc
s.
I didn't update CHANGELOG in #2750. I guess if we did it would go in Deprecations? |
Spoke to @richardjgowers off-github, and his take on this was that changelog changes for this won't be necessary (my bad, should have removed my question yesterday). |
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.
Yeah the thinking is Changelog is a user tool to understand how the package has changed. These changes don’t change anything for the user
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.
Looks good, just a stray six in there. Thank you @IAlibay!
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
- remove six and __future__ imports
Towards #2541
Changes made in this Pull Request:
Question:
Do we want changelog entries for these things?PR Checklist