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.

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

Merged
merged 5 commits into from
Jun 17, 2020
Merged

Conversation

IAlibay
Copy link
Member

@IAlibay IAlibay commented Jun 13, 2020

Towards #2541

Changes made in this Pull Request:

  • Removes six & future imports from lib

Question:

  • Do we want changelog entries for these things?

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@@ -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
Copy link
Member Author

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

@IAlibay
Copy link
Member Author

IAlibay commented Jun 13, 2020

Reverted to raising from None in c23b255, after re-reading #2357 it seemed like this was the behaviour we wanted. Left in the "except *Error as exc" for now, but if we go for None I'm not sure we actually need it.

@IAlibay IAlibay mentioned this pull request Jun 13, 2020
4 tasks
@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #2752 into develop will decrease coverage by 0.17%.
The diff coverage is 92.00%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
package/MDAnalysis/lib/NeighborSearch.py 82.75% <ø> (-0.58%) ⬇️
package/MDAnalysis/lib/__init__.py 100.00% <ø> (ø)
package/MDAnalysis/lib/correlations.py 95.23% <ø> (-0.12%) ⬇️
package/MDAnalysis/lib/formats/__init__.py 100.00% <ø> (ø)
package/MDAnalysis/lib/log.py 76.36% <ø> (-0.43%) ⬇️
package/MDAnalysis/lib/mdamath.py 100.00% <ø> (ø)
package/MDAnalysis/lib/pkdtree.py 89.01% <ø> (-0.12%) ⬇️
package/MDAnalysis/lib/transformations.py 78.41% <ø> (-0.21%) ⬇️
package/MDAnalysis/lib/util.py 86.97% <90.90%> (-0.31%) ⬇️
package/MDAnalysis/lib/distances.py 97.46% <100.00%> (-0.02%) ⬇️
... and 151 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf74d65...f633c79. Read the comment docs.

Copy link
Member

@lilyminium lilyminium left a 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 excs.

package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Outdated Show resolved Hide resolved
@lilyminium
Copy link
Member

I didn't update CHANGELOG in #2750. I guess if we did it would go in Deprecations?

@IAlibay
Copy link
Member Author

IAlibay commented Jun 14, 2020

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

Copy link
Member

@richardjgowers richardjgowers left a 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

@orbeckst orbeckst added the remove-2.0 deprecated in 1.0 and to be removed in 2.0 label Jun 15, 2020
@IAlibay IAlibay requested a review from lilyminium June 16, 2020 06:47
Copy link
Member

@lilyminium lilyminium left a 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!

package/MDAnalysis/lib/util.py Outdated Show resolved Hide resolved
Co-authored-by: Lily Wang <31115101+lilyminium@users.noreply.github.com>
@lilyminium lilyminium merged commit 329c3fc into MDAnalysis:develop Jun 17, 2020
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
- remove six and __future__ imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainability remove-2.0 deprecated in 1.0 and to be removed in 2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants