-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix regression when deps=False is set #187
Conversation
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.
Thanks, @ryanking13! I have one question about the code, which is just for my understanding and won't block merging.
try: | ||
# pyodide-micropip-test depends on snowballstemmer | ||
import snowballstemmer # noqa: F401 | ||
except ModuleNotFoundError: | ||
pass | ||
else: | ||
raise Exception("Should raise!") |
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.
As the intention here is that we want import snowballstemmer
to not be installed due to deps=False
, is there a better way to handle this?
i.e., is it possible to wrap it in a context manager with with pytest.raises(ModuleNotFoundError
(or we have to do it in the current way because pytest
isn't installed?)?
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.e., is it possible to wrap it in a context manager with with pytest.raises(ModuleNotFoundError (or we have to do it in the current way because pytest isn't installed?)?
Yeah, good point. pytest.raises
would work too, but I wanted to keep the test simple, not loading any extra package.
Thanks for the review! |
resolves #180
I forgot to call
await
when deps is not set.