Skip to content
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

Open charts in the default browser with open_editor method #3358

Merged
merged 4 commits into from
Mar 16, 2024

Conversation

joelostblom
Copy link
Contributor

I have been working more in VS Code instead of JupyterLab lately and really come to appreciate the to_url() method added in #3252 (thanks @jonmmease!). VS Code can't correctly handle the Vega-Lite actions menu #2875 and there seems to be little hope of fixing this microsoft/vscode-jupyter#13261.

However, after trying to troubleshoot a few altair specs in the Vega-Lite window, I find myself copying the url over and over again after small edits to the altair spec (for me VS Code only sometimes figures out that the url is a link that can be clicked; maybe related to the length of the spec). It would be convenient if the Vega-Lite spec of the charts could automatically open in the default browser, which is what this PR tries to implement (also mentioned in #3252 (comment)).

I put the browser launch as the default when using to_url(), since I imagine that the most common scenario is to quickly go between the local editor and the online Vega-Editor to troubleshoot a spec. I find this convenient, but if it is considered too aggressive, then I'm happy to change it.

Tagging everyone who was involved in the previous PR discussion: @jonmmease @NickCrews @mattijn @binste

@@ -1104,24 +1104,31 @@ def to_html(
**kwargs,
)

def to_url(self, *, fullscreen: bool = False) -> str:
def to_url(self, *, open_browser: bool = True, fullscreen: bool = False) -> str:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This technically breaks if someone used to rely on fullscreen as a positional argument via to_url(True). I don't think it's a huge deal, but also have nothing against reversing the order.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good thing is that thanks to the *, fullscreen has always been a keyword-only argument so this should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is why we should lean towards kwarg-only APIs when we have the choice :) yay thanks past us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is cool, I haven't come across this use of * to block additional positional args. Thanks for linking the pep!

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Same! I use VS Code a lot and often use .to_url to debug a spec in the Vega Editor. I'm fine with keeping it simple and having open_browser=True as the default. I can't think of another use case for the to_url method but maybe we're missing something?

@joelostblom Could you also add a note in the changelog?
Code looks good to me. Let's see if anyone else has an opinion on this before merging.

@jonmmease
Copy link
Contributor

jonmmease commented Mar 13, 2024

Thanks for the PR @joelostblom, I'm glad the to_url functionality has been helpful!

I think I'm -1 on overloading to_url this way, because I expect to_* methods to perform conversions, not have side effects. And when the goal is to see the chart in the editor, you probably don't want the function to return the full URL and clutter up the notebook output. Also, if goal is to open chart in the editor, the fact that this is done using a URL is more of an implementation detail. e.g. Maybe someday we add the option to create a gist and open the editor pointing there.

So, I'd suggest adding a new top-level chart method named something like chart.open_editor(fullscreen=False) that calls to_url internally and applies the logic in this PR for opening the chart in the browser. We could even expose the other webbrowser.open options in the signature if we want (https://docs.python.org/3/library/webbrowser.html#webbrowser.open). We could also use webbrowser.get to support specifying the name of the browser to use. This doesn't need to be in this PR, just thinking that there are a bunch of additional options folks may want eventually, and so I think a dedicated method is warranted.

@NickCrews
Copy link
Contributor

NickCrews commented Mar 13, 2024

I think I agree with @jonmmease , I'm -.5 on this.

Would it be way too clever if we made it so that to_url() returned a

class VegaUrl(str):
    def open(self): ...

?

This is similar to what some SQL libraries do, they return an object that ducks like a str, but has some additional features.

Using this would look like chart.to_url().open()

@joelostblom
Copy link
Contributor Author

Thanks for the input everyone!

I think I'm -1 on overloading to_url this way, because I expect to_* methods to perform conversions, not have side effects. And when the goal is to see the chart in the editor, you probably don't want the function to return the full URL and clutter up the notebook output. Also, if goal is to open chart in the editor, the fact that this is done using a URL is more of an implementation detail. e.g. Maybe someday we add the option to create a gist and open the editor pointing there.

All great points. I was initially thinking that it seemed unnecessary with yet another method, but after considering what you wrote I think it makes more sense to go down that path.

I don't have a strong preference between chart.open_editor() and chart.to_url().open(). The former seems more discoverable whereas the latter looks neatly composable. What does everyone else think? Two somewhat related points regarding syntax:

  • If we go with the to_url().open() approach, should we also add this to the save method? E.g. chart.save('chart.html').open() could be opened the same way as here (and maybe images/PDFs could also with a file:// URL?).
  • We could think about what the syntax for How to show altair charts from console programs? #3330 should be. I'm guessing we will keep it as chart.show() and not go with something like chart.to_html().open()? We also have chart.display() for displaying via ipython, which doesn't use an .open() approach, so although I quite like the composability, maybe it is too different from what is in Altair already (although we could of course change that if we want).

@joelostblom Could you also add a note in the changelog?

Yup, will do!

@binste
Copy link
Contributor

binste commented Mar 13, 2024

Good points, now I'm also in favour of putting it into a new method :)

I like the custom string classes in SQL libraries! However, I'm not sure if in Altair we'd have a use case other than this for it so it might be a bit unexpected to users and a simple .open_editor could be more straightforward to understand. A URL seems like a general concept which has not much to do with the rest of the library. That's only a slight preference.

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Removing the "Approve" for now as there seems a consensus to use a different method than to_url

@jonmmease
Copy link
Contributor

I prefer open_editor to VegaUrl for discoverability and for the sake of auto-generated API docs, but it is a clever idea!

@joelostblom
Copy link
Contributor Author

Thanks for the input everyone. I also have a slight preference for open_editor for discoverability and consistency with the other methods I mentioned. Since most of us are leaning in that direction I went ahead and updated the PR accordingly. I can't think of any relevant tests to add and we wouldn't want the browser to actually open when running the test suite (maybe there is some clever way around that but probably too much for this small addition?).

@joelostblom joelostblom requested review from NickCrews, binste and jonmmease and removed request for NickCrews March 13, 2024 20:10
@NickCrews
Copy link
Contributor

LGTM!

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

lgtm. Could you update the PR title?

Regarding testing, we could do something clever with monkey patching the webbrowser module. But this is so simple I don't think it's necessary.

@joelostblom joelostblom changed the title Open urls automatically in the default browser with to_url Open charts in the default browser with open_editor method Mar 13, 2024
@joelostblom
Copy link
Contributor Author

@binste Just a ping in case you wanted to have a second look too; otherwise I'm fine to merge.

Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @joelostblom!

@binste binste merged commit bb357e3 into main Mar 16, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants