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 multiple @utils.use_signature issues #3565

Merged
merged 10 commits into from
Sep 6, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Sep 1, 2024

Description

This PR addresses problems with this decorator that cause the wrong arguments to bind in some cases.

I've tried (and failed) to fix this multiple times over the past 1-2 months, but until now, hadn't come up with a solution that:

  • fixed all of the cases
  • satisfied both mypy and pyright

After one signature fails in a chain of methods - an IDE (not ipython) can't understand much that follows.

Therefore, I consider the issue quite signficant.

Tasks

  • Handle methods & functions correctly in use_signature (53057b7)
  • Add a repro for all the call sites this fixes
  • Update use_signature return type
    • pyright is inferring Overload[...], which isn't a form I've seen before
    • Overload[(cb: (T@decorate, ...) -> R@decorate, /) -> ((T@decorate, **P@use_signature) -> R@decorate), (cb: (...) -> R@decorate, /) -> ((**P@use_signature) -> R@decorate)]
    • Whatever the solution is, will most likely be a comment
    • Inserted into docstring to avoid breakage
  • Fix bug w/ Concatenate[T, ...] inside TypeAliasType on < (3, 11)
    • typing.Concatenate for 3.10
    • typing_extensions.Concatenate for 3.8, 3.9

Repro

from typing_extensions import reveal_type
import altair as alt

# Would also include the function equivalent(s),
# if they did not already have `# pyright: ignore`(s)
concat = alt.ConcatChart()
hconcat = alt.HConcatChart()
vconcat = alt.VConcatChart()
layer = alt.LayerChart()

conf_facet = alt.Chart().configure_facet()
conf_concat = alt.Chart().configure_concat()
conf_tooltip_format = alt.Chart().configure_tooltipFormat()
conf_view = alt.Chart().configure_view()

reveal_type(conf_facet)
reveal_type(conf_concat)
reveal_type(conf_tooltip_format)
reveal_type(conf_view)

chained = conf_facet.configure_arc()
reveal_type(chained)
Warnings on main

image

Fixed on fix-use-signature-2

image

Examples that fail type checking on main

Screenshots

Issues in api.py

I added these ignore comments in response to this issue during 72bd84d

return ConcatChart(concat=charts, **kwargs) # pyright: ignore

return HConcatChart(hconcat=charts, **kwargs) # pyright: ignore

return VConcatChart(vconcat=charts, **kwargs) # pyright: ignore

return LayerChart(layer=charts, **kwargs) # pyright: ignore

image

Enabling type checking for tests/

image

@dangotbanned dangotbanned marked this pull request as ready for review September 2, 2024 13:46
@dangotbanned dangotbanned requested a review from binste September 2, 2024 14:02
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.

Nice! A lot for me to learn here. I've added some questions where I could use your help so I can review this further.

altair/utils/core.py Show resolved Hide resolved
altair/utils/core.py Show resolved Hide resolved
@dangotbanned
Copy link
Member Author

Nice! A lot for me to learn here. I've added some questions where I could use your help so I can review this further.

Thanks for the review @binste!

Tried my best to answer everything so far.
This was definitely quite a complex one to solve, so I'm happy to explain more if you have any other questions.

@dangotbanned dangotbanned enabled auto-merge (squash) September 5, 2024 13:53
@dangotbanned dangotbanned requested a review from binste September 6, 2024 07:25
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.

Thanks again for digging through it! :)

@dangotbanned dangotbanned merged commit a7c227b into vega:main Sep 6, 2024
13 checks passed
@dangotbanned
Copy link
Member Author

Thanks again for digging through it! :)

No problem, thanks for taking the time to review @binste

@dangotbanned dangotbanned deleted the fix-use-signature-2 branch September 6, 2024 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants