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

simplify deprecation message for altair.themes? #3694

Closed
mattijn opened this issue Nov 23, 2024 · 6 comments · Fixed by #3695
Closed

simplify deprecation message for altair.themes? #3694

mattijn opened this issue Nov 23, 2024 · 6 comments · Fixed by #3695

Comments

@mattijn
Copy link
Contributor

mattijn commented Nov 23, 2024

What is your suggestion?

In the deprecation message of altair.themes we see currently among other things:

" def custom_theme() -> alt.theme.ThemeConfig:\n"

    # Updated
    @alt.theme.register('theme_name', enable=True)
    def custom_theme() -> alt.theme.ThemeConfig:
        return {'height': 400, 'width': 700}

I'm wondering if we really need to add -> alt.theme.ThemeConfig when defining this function. Within a Jupyter Notebook I really rarely see this approach being utilised and in combination with the usage of a @ decorator to register the theme, also something I really rarely see people doing a Jupyter Notebook, this deprecation message feels a bit overwhelming to me.

Have you considered any alternative solutions?

@dangotbanned, if it's not too much trouble, I prefer making one change at a time as a user. Can we just do this?

# Updated
@alt.theme.register('theme_name', enable=True)
def custom_theme():
    return {'height': 400, 'width': 700}
@dangotbanned
Copy link
Member

dangotbanned commented Nov 23, 2024

@mattijn I do understand the concern.

One thing I'd add though is that the annotation is what provides the static typing feedback.

So even if you don't understand typing, if you copy and paste the code - your IDE can let you know if something is wrong.

Original

import altair as alt

@alt.theme.register('theme_name', enable=True)
def custom_theme() -> alt.theme.ThemeConfig:
    return {'height': 400, 'width': 700}

Alternatives

If we just want to shorten it we could say:

from altair import theme

@theme.register('theme_name', enable=True)
def custom_theme() -> theme.ThemeConfig:
    return {'height': 400, 'width': 700}

or

from altair.theme import register, ThemeConfig

@register('theme_name', enable=True)
def custom_theme() -> ThemeConfig:
    return {'height': 400, 'width': 700}

Both of these would be a shorter message - while not limiting the benefits of the original

@mattijn
Copy link
Contributor Author

mattijn commented Nov 23, 2024

I'm in JupyterLab and I don't get any feedback🤷‍♂️

@dangotbanned
Copy link
Member

dangotbanned commented Nov 23, 2024

I'm in JupyterLab and I don't get any feedback🤷‍♂️

I think we'll need to figure out a good suggestion for that case as part of #3645

Non-typing users

  • Class-syntax is required for completions
  • Still likely an improvement over using a dict
  • Point to resources on how they can get the full benefits

For example https://github.com/jupyter-lsp/jupyterlab-lsp

Edit: Looks more complicated than adding an extension in VSCode

@mattijn
Copy link
Contributor Author

mattijn commented Nov 23, 2024

Can we do this?

import altair as alt

@alt.theme.register('theme_name', enable=True)
def custom_theme():
    return alt.theme.ThemeConfig(
        {'height': 400, 'width': 700}
    )

Still a single import altair as alt import and also just a dictionary as input of the alt.theme.ThemeConfig, an object that you have when facing this message.

@dangotbanned
Copy link
Member

dangotbanned commented Nov 23, 2024

#3694 (comment)

Yeah @mattijn that works for me

You can also use keyword args if you're using ThemeConfig directly.
Not sure if that would be preferred but thought I'd mention as an option

@mattijn
Copy link
Contributor Author

mattijn commented Nov 23, 2024

Once JupyterLab and Jupyter Notebook (classic) have proper language service support, these types of changes will be easier to implement. However, we're not there yet. Thanks for helping to find an intermediate solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants