raise error for sidecar or popout when not supported #3309
Conversation
| @@ -126,6 +127,8 @@ def show_widget(widget, loc, title): # pragma: no cover | |||
| display(widget) | |||
|
|
|||
| elif loc.startswith('sidecar'): | |||
| if not get_kernel_id(): | |||
There was a problem hiding this comment.
@mariobuikhuizen - is this sufficient, or would accessing getBaseUrl() be more appropriate (in which case can that be exposed back to python in ipypopout)?
There was a problem hiding this comment.
I don't think there is a 1 on 1 mapping of where ipypopout and sidecar work. I think sidecar has different requirements than ipypopout.
There was a problem hiding this comment.
do you have any suggestions on which conditions to use for each for now then?
There was a problem hiding this comment.
I don't know how to detect if sidecar is available.
There was a problem hiding this comment.
ok, then I think this is better than nothing for now and we can always revise the condition going forward 🤷♂️
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3309 +/- ##
=======================================
Coverage 88.81% 88.81%
=======================================
Files 125 125
Lines 19036 19037 +1
=======================================
+ Hits 16906 16907 +1
Misses 2130 2130 ☔ View full report in Codecov by Sentry. |
|
The error messages get triggered for me as expected. I noticed one or two other interesting things happening when running in VSCode, do we want to (or did we already?) add a disclaimer in the docs that running in VSCode is not fully supported? |
|
I was also able to reproduce the error for the default python environment and in the case of if I set it to my normal work environment. I did find a case that if you have a jupyter server running independent of VSCode, you can select the option for an existing Jupyter server, which then will result in an existing kernel id and the error does not get raised. |
|
@rosteen - can you please create tickets for anything you found? We can then discuss whether we should add a warning or not (I don't think we advertise anywhere that we do support VSCode, but if its sufficiently broken, we should either fix it or maybe make a note in the docs). @gibsongreen - ok, this probably is still a step in the right direction, but maybe there is a better condition to test. @mariobuikhuizen - did you have any ideas or should we just move forward with this PR to at least cover the most common scenario? |
See #3313, I'll see if I can find anything else. |
kecnry
requested review from
rosteen,
javerbukh,
pllim,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
There was a problem hiding this comment.
With the conversation in mind, this looks good to me!
I added a video to #3313 with additional related behavior. In the video it also shows unrelated issues in which icons are unable to display throughout the app and axes become difficult to read in dark-mode. This didn't occur when testing Imviz and when I tested before in Cubeviz I saw both expected and bug behavior. I'm in favor in a separate effort of adding a docs disclaimer along with a note for the user to open issues.
…en not supported
…ed (#3324) Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>
Description
This pull request raises an error when trying to show a widget in popout or sidecar when they are not supported (i.e. in VS Code). Related to #3251.
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.