Skip to content

made GSD windows support optional, now raises Error#2090

Merged
tylerjereddy merged 1 commit intodevelopfrom
issue-1923-gsdwindows
Oct 8, 2018
Merged

made GSD windows support optional, now raises Error#2090
tylerjereddy merged 1 commit intodevelopfrom
issue-1923-gsdwindows

Conversation

@richardjgowers
Copy link
Member

Fixes #1923

Changes made in this Pull Request:

  • now optionally import GSD modules

PR Checklist

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

@richardjgowers
Copy link
Member Author

@tylerjereddy I don't have a windows box, but I think this should work.. some tests will need to be marked as xfail for windows (if they're not already)

Copy link
Member

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Looks like the os checks are reversed -- interestingly, I only had to fix one of them, in topology path, to get tests to pass on win locally, but probably best to adjust both.

from . import null
from . import dummy

if not os.name == 'nt':
Copy link
Member

Choose a reason for hiding this comment

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

You probably want != here

from . import GSDParser
from . import MinimalParser

if not os.name == 'nt':
Copy link
Member

Choose a reason for hiding this comment

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

Probably want != here

@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

Merging #2090 into develop will decrease coverage by <.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2090      +/-   ##
===========================================
- Coverage    89.41%   89.41%   -0.01%     
===========================================
  Files          159      159              
  Lines        18731    18737       +6     
  Branches      2696     2698       +2     
===========================================
+ Hits         16749    16753       +4     
  Misses        1381     1381              
- Partials       601      603       +2
Impacted Files Coverage Δ
package/MDAnalysis/topology/GSDParser.py 84% <75%> (-1.11%) ⬇️
package/MDAnalysis/coordinates/GSD.py 85.1% <80%> (-1.26%) ⬇️

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 edb1643...be2d47e. Read the comment docs.

@richardjgowers
Copy link
Member Author

Ok my last fix wasn't any good as it changed the namespace (ie MDAnalysis.coordinates.GSD.GSDReader wouldn't exist) so it might break scripts rather than fail gracefully as intended. This fix is even more fixier than before.

Copy link
Member

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

LGTM now -- I don't think we need to worry too much about the coverage failures; I suppose we could check for the exception raising on windows if we really wanted.

Once this is merged I think we can remove the gsd pip install from appveyor; this may also mean that 2.7 becomes tractable on Windows too since I think issues there were related primarily to gsd.

@tylerjereddy
Copy link
Member

Ok, in it goes. I think we can safely ignore codecov patch diff coverage being a little off target here.

This should mean we can safely remove gsd installs to Windows builds, which were only acting as "mock" imports really.

@tylerjereddy tylerjereddy merged commit 80560da into develop Oct 8, 2018
@tylerjereddy tylerjereddy deleted the issue-1923-gsdwindows branch October 8, 2018 02:37
@bdice bdice mentioned this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants