-
-
Notifications
You must be signed in to change notification settings - Fork 108
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 logic for no geoviews explorer #1451
base: main
Are you sure you want to change the base?
Conversation
ooh that's fancy and good to know! Turns out there actually was a marker for |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1451 +/- ##
==========================================
+ Coverage 88.94% 89.03% +0.08%
==========================================
Files 52 52
Lines 7781 7794 +13
==========================================
+ Hits 6921 6939 +18
+ Misses 860 855 -5 ☔ View full report in Codecov by Sentry. |
Are you sure? |
It's also a little crazy right 🙃 A suggestion could be to refactor |
Hm I guess there is a marker, but the tests still run everything I guess I'll go with Maxime's suggestion |
7d8b05e
to
bae3bd7
Compare
gv_available = find_spec('geoviews') is not None | ||
|
||
if not gv_available and raise_error: | ||
raise ImportError('GeoViews must be installed to enable the geographic options.') |
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 got confused a minute here, does find_spec('geoviews')
actually import GeoViews?
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.
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.
Yes ok that's what I thought. I was confused as then an ImportError
is raised. Since importing GeoViews comes with side effects (registering lots of stuff), I'd like to make sure we're not removing an important step there. @hoxbro did you suggest find_spec
for performance reasons?
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.
Just as an alternative and also easier to patch in the test suite.
I think it is okay to use find_spec
when we check if it is available. We should import geoviews in the code, where there previously was a try/except
, to get those juicy side effects.
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 also think the error message is wrong here. As it no longer only affects the UI code.
Even though
geo=False
, explorer would raiseraise ImportError('GeoViews must be installed to enable the geographic options.')
Not sure how to add a test for this because the dev dependencies installs geoviews.