Skip to content

fix(typing): Revise generated annotation order #3655

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

Merged
merged 5 commits into from
Oct 23, 2024
Merged

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Oct 23, 2024

Description

The doc stated that None would appear after builtins.

def sort_type_reprs(tps: Iterable[str], /) -> list[str]:
"""
Shorter types are usually the more relevant ones, e.g. `str` instead of `SchemaBase`.
We use `set`_ for unique elements, but the lack of ordering requires additional sorts:
- If types have same length names, order would still be non-deterministic
- Hence, we sort as well by type name as a tie-breaker, see `sort-stability`_.
- Using ``str.lower`` gives priority to `builtins`_ over ``None``.
- Lowest priority is given to generated aliases from ``TypeAliasTracer``.
- These are purely to improve autocompletion

However, this assertion doesn't hold for any builtin with a name length greater than 4.

The issue is not new, but became more apparent to me while writing #3653 (comment)

Example

Below float appears after None

Before

tooltip: str | bool | None | float | TooltipContentKwds

After

tooltip: str | bool | float | TooltipContentKwds | None

Note

I was unable to work out a sorting strategy to achieve what the doc had intended.

Instead, I've opted to always place None last in the context it appears.
This is much easier to do, and is generally what most people would write (and expect to see).

In the non-generated code, this is already the pattern we use:

format: Literal["json", "html", "png", "svg", "pdf"] | None = None,

def _repr_mimebundle_(self, *args, **kwds) -> MimeBundleType | None: # type:ignore[return] # noqa: ANN002, ANN003

self, row_limit: int | None = None, exclude: Iterable[str] | None = None

Preventing regressions

I've started small, but have included a few doctests (f9f0a53) that are now run alongside (20d5c17) those in altair, tests.

The doc stated that `None` would appear after builtins, but any builtin with a length greater than 4 appeared after.

The issue is not new, but became more apparent to me while writing #3653 (comment)
Bit of a driveby commit, but I want to enable doctests on `tools`.
These were blocking for that
@dangotbanned
Copy link
Member Author

Pinging @binste so that you see this when you're available again.

Happy to revert later if you have any objections 👍

@dangotbanned dangotbanned marked this pull request as ready for review October 23, 2024 17:42
@dangotbanned dangotbanned changed the title fix(typing): Revise annotation sorting fix(typing): Revise generated annotation order Oct 23, 2024
@dangotbanned dangotbanned merged commit f373373 into main Oct 23, 2024
23 checks passed
@dangotbanned dangotbanned deleted the fix-sort-type-none branch October 23, 2024 17:51
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.

1 participant