-
-
Notifications
You must be signed in to change notification settings - Fork 489
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 viewable show keyword arguments #1106
Conversation
panel/viewable.py
Outdated
@@ -244,7 +244,7 @@ def servable(self, title=None): | |||
return self | |||
|
|||
def show(self, port=0, websocket_origin=None, threaded=False, | |||
title=None, verbose=True, **kwargs): | |||
title=None, verbose=True, show=False, **kwargs): |
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 don't see the show
argument documented in the show()
method docstring, which seems important given how unintuitive show(show=False)
is. :-)
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.
Added the docstring and changed the default to the previous behaviour.
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 copied the docstring from server.py
. Should kwarg show be popup or open?
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.
Oops; responded in different thread, proposing open
or launch
or browse
. Probably should let @philippjfr decide.
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.
No strong preference but I vote for open
and I'd probably only rename it here.
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.
show(show=True)
does look somewhat absurd (but I really want you to show...). I think the votes are in.
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.
Or even more strange to request show(show=False)
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 will push open
but feel free to revert (open
has the disadvantage of being a python command as well).
Looks good thanks, those py2 CI issues will be resolved separately. |
keyword arguments show and title are now passed to the get_server.