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

Improve sphinx.util.typing [part 2] #12257

Closed
wants to merge 20 commits into from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Apr 10, 2024

This one is built on top of #12256. It would help when I want to support typing_extensions.Unpack. It also cleans up some old code that, honestly, was hard to read.

Oh by the way, there was a bug here:

        elif UnionType and isinstance(cls, UnionType):
            if len(cls.__args__) > 1 and None in cls.__args__:
                args = ' | '.join(restify(a, mode) for a in cls.__args__ if a)
                return 'Optional[%s]' % args

This portion of the code would never be evaluated since cls.__args__ contains types. On the other hand,

        elif (inspect.isgenericalias(cls)
              and cls.__module__ == 'typing'
              and cls.__origin__ is Union):  # type: ignore[attr-defined]
            if (len(cls.__args__) > 1  # type: ignore[attr-defined]
                    and cls.__args__[-1] is NoneType):  # type: ignore[attr-defined]
                if len(cls.__args__) > 2:  # type: ignore[attr-defined]
                    args = ', '.join(restify(a, mode)
                                     for a in cls.__args__[:-1])  # type: ignore[attr-defined]
                    return ':py:obj:`~typing.Optional`\\ [:obj:`~typing.Union`\\ [%s]]' % args
                else:
                    return ':py:obj:`~typing.Optional`\\ [%s]' % restify(
                        cls.__args__[0], mode)  # type: ignore[attr-defined]
            else:
                args = ', '.join(restify(a, mode)
                                 for a in cls.__args__)  # type: ignore[attr-defined]
                return ':py:obj:`~typing.Union`\\ [%s]' % args

but this would render Union[None, int] as Union[None, int] instead of Optional[int]. For Union types written in | style, I will keep the order so that it is consistent with Python 3.10 representation (int | None is printed as is and None | int as None | int). For old-style unions, I'm not sure whether I should only put Optional if None is at the end or put Optional for the whole Union so I'm open to suggestions (for now, it just removes the None-like values, make them an Union, and put an optional around it).

Note: Literal[None] are kept as is since they are also rendered differently in native Python (int | Literal[None] == Union[int, Literal[None]] vs int | None == int | None)

@picnixz picnixz changed the title [cleanup] improve sphinx.util.typing [cleanup] improve sphinx.util.typing [part 2] Apr 10, 2024
sphinx/util/inspect.py Show resolved Hide resolved
sphinx/util/inspect.py Show resolved Hide resolved
sphinx/util/inspect.py Outdated Show resolved Hide resolved
sphinx/util/typing.py Outdated Show resolved Hide resolved
sphinx/util/typing.py Show resolved Hide resolved
@picnixz picnixz requested a review from AA-Turner April 13, 2024 10:04
@picnixz picnixz changed the title [cleanup] improve sphinx.util.typing [part 2] Improve sphinx.util.typing [part 2] Apr 13, 2024
@AA-Turner
Copy link
Member

I haven't had as much time as I had hoped, so I'll outline some thoughts rather than pushing an update to the PR.

I don't think this is in a state to merge, currently. Further, I think it tries to do too many things, and is confusing in so doing.

I would prefer to have distinct PRs introducing the TypeGuard changes, mock changes in autodoc tests, sentinel changes in util.inspect, stylistic refactors, and the bug fix.

It's perhaps a bit overkill to have 5 PRs in total, but at least a good degree of clarity in the individual commits such that a reviewer can follow along would be a first step -- it is currently hard to verify what each refactor does as so much of the line changes in the commit, and this hard to verify that the changes don't have any unintended effects or logical errors etc.

Sorry for the less immediately positive feedback. I've started on these changes locally, but realistically won't be able to finish them before Sunday.

A

@picnixz
Copy link
Member Author

picnixz commented Apr 13, 2024

I would prefer to have distinct PRs introducing the TypeGuard changes, mock changes in autodoc tests, sentinel changes in util.inspect, stylistic refactors, and the bug fix.

Oh it's fine. I can do the typeguards separately. There are some changes that need to be synced because of the tests but I think I can postpone the bug fix itself and its refactorization.

I just needed something to support typing_extensions.Unpack and since it didn't take me too much time I went brr in 1 PR. Now, I'm in the middle of writing my thesis so I'll be less present until late may (my deadline).

@picnixz
Copy link
Member Author

picnixz commented Apr 13, 2024

I think I'll do:

  • the typing.Any bugfix for the test (totally independent of everything else)
  • the bug fix about None
  • the type guards + the remaining typing (I need to do it in one go since mypy complains otherwise and I don't want to add type: ignore if it's just to remove them one PR after)
  • the rest of the refactorization

@picnixz picnixz marked this pull request as draft April 13, 2024 15:51
@AA-Turner
Copy link
Member

In terms of the 7.3 release, I've pushed a subset of the changes here but I think best to leave the rest for later.

A

@picnixz
Copy link
Member Author

picnixz commented Apr 15, 2024

I actually separated the PR yesterday locally so I'll just open them today

@picnixz
Copy link
Member Author

picnixz commented Apr 15, 2024

Superseeded by #12280, #12281, #12282, #12283 and #12284.

@picnixz picnixz closed this Apr 15, 2024
@picnixz picnixz deleted the cleanup/util-2-typing branch April 16, 2024 08:53
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants