Skip to content

No error if import fails unconditionally#108

Merged
bsipocz merged 2 commits intoscientific-python:masterfrom
pllim:no-import-no-cry
May 4, 2020
Merged

No error if import fails unconditionally#108
bsipocz merged 2 commits intoscientific-python:masterfrom
pllim:no-import-no-cry

Conversation

@pllim
Copy link
Contributor

@pllim pllim commented Apr 30, 2020

A hacky but more global fix than astropy/astropy#10242 ? 😬

@pllim pllim requested review from astrofrog, bsipocz and saimn April 30, 2020 18:24
@pllim pllim added the 🔥 Critical label Apr 30, 2020
Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

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

Was about to push the same thing, so yes looks good. Probably the best thing to do for now.

@bsipocz
Copy link
Member

bsipocz commented Apr 30, 2020

I think I've changed my opinion since this morning, and now having this worked around in astropy seems a more right solution, after all collection without optional dependencies should work there, thus this "feature" recovered a few bugs already.

@pllim
Copy link
Contributor Author

pllim commented May 1, 2020

As discussed offline, I have no strong feeling either way.

@astrofrog
Copy link
Contributor

I think the correct fix is to collect doctests without importing files, instead e.g. parsing files with ast or something similar to find docstrings.

@pllim
Copy link
Contributor Author

pllim commented May 1, 2020

I think the correct fix is to collect doctests without importing files, instead e.g. parsing files with ast or something similar to find docstrings.

How does the doctest that ships with pytest do it? Will we risk deviating too much from pytest?

@bsipocz
Copy link
Member

bsipocz commented May 1, 2020

pytest's doctest does the exact same we had here for 0.6.0 in #103:

                module = self.fspath.pyimport()
            except ImportError:
                if self.config.getvalue("doctest_ignore_import_errors"):
                    pytest.skip("unable to import module %r" % self.fspath)
                else:
                    raise

@pllim
Copy link
Contributor Author

pllim commented May 1, 2020

Sorry, my question in #108 (comment) is a reply to #108 (comment) . (I clarified my comment above.)

@bsipocz
Copy link
Member

bsipocz commented May 1, 2020

oh, I assumed the first one was a question.

@bsipocz
Copy link
Member

bsipocz commented May 4, 2020

Let's go ahead with this one for now, and maybe #110 for the future.

I'm merging this now, but they may be some more outstanding issues, as using this with pytest 5.4, there are still asdf related ModuleNotFoundErrors popping up.

@bsipocz bsipocz force-pushed the no-import-no-cry branch from e8ade44 to 7fc0c9a Compare May 4, 2020 08:19
@bsipocz bsipocz merged commit 2c604a0 into scientific-python:master May 4, 2020
@bsipocz
Copy link
Member

bsipocz commented May 4, 2020

Thank you @pllim!

@pllim pllim deleted the no-import-no-cry branch May 4, 2020 14:40
@pllim
Copy link
Contributor Author

pllim commented May 4, 2020

I haven't caught up with all the notifications yet but I am hoping that maybe astropy/astropy#10260 will handle it. If not, perhaps the last resort is to have setup.cfg ignore it on the astropy side. 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔥 Critical

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants