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

Enable doctests #263

Merged
merged 13 commits into from
Jun 10, 2019
Merged

Enable doctests #263

merged 13 commits into from
Jun 10, 2019

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Jun 5, 2019

On current master there are 6 error and 17 failures when doctests are enabled. This PR fixes the errors, and I feel that we should merge in commits from here with the exception (in case there are still failures) of the first to the upcoming release.

@bsipocz bsipocz added this to the 0.4 milestone Jun 5, 2019
@bsipocz
Copy link
Member Author

bsipocz commented Jun 6, 2019

Locally this passes.

@cdeil
Copy link
Member

cdeil commented Jun 6, 2019

There's CI fails here: https://travis-ci.org/astropy/regions/jobs/542065875#L1458

And one fail here: https://ci.appveyor.com/project/Astropy/regions/builds/25078976/job/cmhckttgxl0t663y#L406

@bsipocz - Can you find a way to avoid those?

Is it possible to skip the doctests on those builds?
(I mean, spending time on repr differences across versions isn't fun - avoid it if you can)

@cdeil
Copy link
Member

cdeil commented Jun 6, 2019

Alternatively, you could also turn doctest off again here and we merge all these edits / fixes immediately. The diff is large, this should go in quickly.

This was referenced Jun 6, 2019
@bsipocz bsipocz force-pushed the doctest_enable_them branch 10 times, most recently from 59df46f to b433891 Compare June 9, 2019 02:01
@bsipocz
Copy link
Member Author

bsipocz commented Jun 9, 2019

If we really want to test on 32bit, then the image needs to be updated. Currently I'm unable to install mpl on it as there are no wheel, and I cannot yum install a recent enough freetype. Falling back on ancient mpl that still has the wheels.

@bsipocz
Copy link
Member Author

bsipocz commented Jun 9, 2019

(another alternative though to make mpl an optional dependency for the docs, too).

@bsipocz
Copy link
Member Author

bsipocz commented Jun 9, 2019

There are some issues with the appveyor 2.7 build when mpl is installed. Rather than spending more time in the rabbit hole I removed that job given that we plan to remove python2 support after this release.

Note that everything was fine before this PR, so I'm confident things are not worse that they would have been without the doctests being run.

@bsipocz
Copy link
Member Author

bsipocz commented Jun 9, 2019

I've run out of ideas about the appveyor build. Everything looks the same as for photutils and yet it doesn't work. And in addition, it used to work before this PR.

@bsipocz bsipocz force-pushed the doctest_enable_them branch 2 times, most recently from 9227fc4 to 0534d60 Compare June 9, 2019 20:32
@bsipocz
Copy link
Member Author

bsipocz commented Jun 10, 2019

All green but with the caveat that the appveyor jobs are allowed to fail. I suggest that while they are being allowed to fail is to have extra case and look at the log of the seemingly passing builds before merging feature PRs.

@bsipocz
Copy link
Member Author

bsipocz commented Jun 10, 2019

(out of band discussion with @cdeil, we agreed that I merge this asap I get a green status, so he can proceed with releasing 0.4)

@bsipocz bsipocz merged commit 7bd233d into astropy:master Jun 10, 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