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

Issues with trait typing #863

Closed
blink1073 opened this issue Sep 14, 2023 · 12 comments · Fixed by #868
Closed

Issues with trait typing #863

blink1073 opened this issue Sep 14, 2023 · 12 comments · Fixed by #868

Comments

@blink1073
Copy link
Contributor

blink1073 commented Sep 14, 2023

In #818 we introduced typing of traits. While testing the feature with nbconvert, I saw two issues relating to overloads:

nbconvert/nbconvertapp.py:331: error: No overload variant of "Unicode" matches argument types "bool", "str"  [call-overload]
nbconvert/nbconvertapp.py:331: note: Possible overload variants:
nbconvert/nbconvertapp.py:331: note:     def [G, S] __init__(self, default_value: Union[str, Sentinel] = ..., allow_none: Literal[False] = ..., read_only: Optional[bool] = ..., help: Optional[str] = ..., config: Any = ..., **kwargs: Any) -> Unicode[str, Union[str, bytes]]
nbconvert/nbconvertapp.py:331: note:     def [G, S] __init__(self, default_value: Union[str, Sentinel, None] = ..., allow_none: Literal[True] = ..., read_only: Optional[bool] = ..., help: Optional[str] = ..., config: Any = ..., **kwargs: Any) -> Unicode[Optional[str], Union[str, bytes, None]]

The source code was:

    export_format = Unicode(
        allow_none=False,
        help=f"""The export format to be used, either one of the built-in formats
        {get_export_names()}
        or a dotted object name that represents the import path for an
        ``Exporter`` class""",
    ).tag(config=True)

And:

nbconvert/preprocessors/sanitize.py:52: error: No overload variant of "Any" matches argument types "bool", "Dict[str, List[str]]", "str"  [call-overload]
nbconvert/preprocessors/sanitize.py:52: note: Possible overload variants:
nbconvert/preprocessors/sanitize.py:52: note:     def __init__(self, default_value: str = ..., *, allow_none: Literal[False], read_only: Optional[bool] = ..., help: Optional[str] = ..., config: Optional[Any] = ..., **kwargs: Any) -> traitlets.traitlets.Any
nbconvert/preprocessors/sanitize.py:52: note:     def __init__(self, default_value: str = ..., *, allow_none: Literal[True], read_only: Optional[bool] = ..., help: Optional[str] = ..., config: Optional[Any] = ..., **kwargs: Any) -> traitlets.traitlets.Any
nbconvert/preprocessors/sanitize.py:52: note:     def __init__(self, default_value: str = ..., *, allow_none: Literal[True, False] = ..., help: Optional[str] = ..., read_only: Optional[bool] = ..., config: Any = ..., **kwargs: Any) -> traitlets.traitlets.Any

Where the source code was:

    attributes = Any(
        config=True,
        default_value=ALLOWED_ATTRIBUTES,
        help="Allowed HTML tag attributes",
    )

We should use these failures as test cases with the fixes.

@blink1073
Copy link
Contributor Author

cc @maartenbreddels

@blink1073
Copy link
Contributor Author

blink1073 commented Sep 14, 2023

Oh, and one more issue, Set is being interpreted as List:

    remove_cell_tags = Set(
        Unicode(),
        default_value=[],
        help=(
            "Tags indicating which cells are to be removed,"
            "matches tags in ``cell.metadata.tags``."
        ),
    ).tag(config=True)
nbconvert/preprocessors/tagremove.py:78: error: "List[Any]" has no attribute "intersection"  [attr-defined]

@blink1073
Copy link
Contributor Author

Overall this is a positive change and I don't think we should revert based on these shortcomings.

@blink1073
Copy link
Contributor Author

blink1073 commented Sep 14, 2023

Another one: error: Call to untyped function "List" in typed context [no-untyped-call]

    latex_command = List(
        ["xelatex", "{filename}", "-quiet"], help="Shell command used to compile latex."
    ).tag(config=True)

@blink1073
Copy link
Contributor Author

Nice to fix: error: Call to untyped function "Dict" in typed context [no-untyped-call]

_exporters = Dict()

@blink1073
Copy link
Contributor Author

Config should be a dict of type str, typing.Any error: Call to untyped function "Config" in typed context [no-untyped-call]

@blink1073
Copy link
Contributor Author

error: Call to untyped function "Union" in typed context [no-untyped-call]

    style = Union(
        [Unicode("default"), Type(klass=Style)],
        help="Name of the pygments style to use",
        default_value=JupyterStyle,
    ).tag(config=True)

@blink1073
Copy link
Contributor Author

error: Call to untyped function "Set" in typed context [no-untyped-call]

    safe_output_keys = Set(
        config=True,
        default_value={
            "metadata",  # Not a mimetype per-se, but expected and safe.
            "text/plain",
            "text/latex",
            "application/json",
            "image/png",
            "image/jpeg",
        },
        help="Cell output mimetypes to render without modification",
    )

@blink1073
Copy link
Contributor Author

Our helper functions should have type hints:

error: Call to untyped function "import_item" in typed context [no-untyped-call]

@bollwyvl
Copy link
Contributor

As these issues get resolved, it's probably worth adding some guidance to the docs. A specific case: will it be possible for downstreams to use the if TYPE_CHECKING and string-quoted annotations to support 5.9 and 5.10?

Also noted over on #864 (comment), is there a future where annotated-types could be used, so these types are less insular?

@blink1073
Copy link
Contributor Author

I don't see how the specific types exposed in annotated-types are relevant to traitlets, but perhaps Annotation in general could be used. I don't have bandwidth to work on such an effort.

I view what we had prior to 5.10 as wholly deficient, and not worth supporting from a typing perspective.

For example, the ipykernel typing tests are now properly alerting that we aren't handling None in many places.

@maartenbreddels
Copy link
Contributor

Hi @blink1073 ,

Thank you for opening the issue!. i'm currently traveling, so I cannot work on this now. Nice to see this is already catching the None cases!

Regards,

Maarten

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 a pull request may close this issue.

3 participants