fix: Resolve alt.binding signature/docstring issues#3671
Merged
dangotbanned merged 6 commits intomainfrom Nov 3, 2024
Merged
Conversation
- `Binding` is not generic, it is a union of all bindings - It has no doc or parameters - `binding()` currently forces this function to be `BindInput` as `input` is a required argument
- Discovered the backwards incompatibility during testing - Manually spelling out the signature & copying the docs; was the only reliable method I found
- Ensures new signature is fully backwards compatible - Demonstrates (previously) inconsistent typing behavior that is now resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed a few problems with
alt.bindingduring #3544.Bug
We have one example (scatter_point_paths_hover) that uses the function.
Code reference
altair/tests/examples_arguments_syntax/scatter_point_paths_hover.py
Line 33 in 88c76e3
altair/tests/examples_methods_syntax/scatter_point_paths_hover.py
Line 33 in 88c76e3
pyright- but notmypy- thinks there is an issue here and displays this warning:Since the warning is being emitted - despite the example working as intended - I'd consider this a false-negative.
Besides the warning, you can see that:
This is not purely an issue with
pyright, but can be seen in the API Reference.Fix
The tests added in (0266b8d) demonstrate most of what is fixed.
However, a much bigger improvement can be seen in the now-meaningful signature and docstring for
alt.binding:These benefits will also be present in the
5.5.0API Reference