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

fix(typing): Resolve mypy==1.11.0 issues in plugin_registry #3487

Merged
merged 2 commits into from
Jul 20, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 20, 2024

Fixes

altair/utils/plugin_registry.py:213: error: "PluginType" not callable  [misc]
altair/utils/plugin_registry.py:213: note: Error code "misc" not covered by "type: ignore" comment
Found 1 error in 1 file (checked 369 source files)
Error: Process completed with exit code 1.

Description

The release of Mypy 1.11 introduced stricter typing for functools.partial.
The existing # type: ignore comments were no longer sufficient and correcting everything (whilst not breaking all the derived registries) was more complex than I'd like.
Hopefully this summary is easier to understand than the diff alone.

Fix

The fix is mostly 2 changes:

  1. Propagating the return type of plugin functions
  2. Switching from assert isinstance(...) to a narrowing function

For 2, I have added a courtesy deprecation.
No current altair code (including tests) used the plugin_type argument.
I don't think PluginRegistry is intended to be public as it doesn't appear in alt.__all__.

The new default has identical behaviour to the previous default
It is also safer as it won't be disabled by optimizations & better adheres to the current typing spec guidance.

R = TypeVar("R")
Plugin = TypeAliasType("Plugin", Callable[..., R], type_params=(R,))
PluginT = TypeVar("PluginT", bound=Plugin[Any])
IsPlugin = Callable[[object], TypeIs[Plugin[Any]]]
Copy link
Member Author

@dangotbanned dangotbanned Jul 20, 2024

Choose a reason for hiding this comment

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

As an example of how this could be used.

@runtime_checkable would not be useful as it doesn't check types. Any callable would pass, regardless of the return type.

alt.utils.theme.py

from .plugin_registry import PluginRegistry
from typing import Callable
from typing_extensions import TypeAlias, TypeIs

ThemeType: TypeAlias = Callable[..., dict]

def is_theme_plugin(obj: Callable[..., Any]) -> TypeIs[ThemeType]:
    from inspect import signature
    from typing import get_origin

    sig = signature(obj)
    ret = sig.return_annotation
    return ret is dict or get_origin(ret) is dict

class ThemeRegistry(PluginRegistry[ThemeType, dict]):
    pass

alt.vegalite.v5.theme.py

from typing import Final
from ...utils.theme import ThemeRegistry, is_theme_plugin

ENTRY_POINT_GROUP: Final = "altair.vegalite.v5.theme"
themes = ThemeRegistry(entry_point_group=ENTRY_POINT_GROUP, plugin_type=is_theme_plugin)

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, thanks for the fix!

@binste binste merged commit b1b8c6e into vega:main Jul 20, 2024
14 checks passed
@dangotbanned dangotbanned deleted the fix-mypy-111-plugin-reg branch July 21, 2024 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants