-
Notifications
You must be signed in to change notification settings - Fork 796
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
Type hints: Parts of folders "vegalite", "v5", and "utils" #2976
Merged
Merged
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
4146bba
Type hint vegalite/v5/theme.py
binste cc17fa2
Type hint display modules
binste e748ce0
Type hint data modules and some related files
binste 7feb6ab
Type hint core.py
binste dd5694a
Type hint html.py
binste e1693e5
Format code
binste 474d693
Merge branch 'master' into type_hint_vl_v5_files
binste 3f2c220
Type hint default_data_transformer
binste cb2a8a8
Improve type hints on data transformers
binste 570a358
Ignore mypy missing import error for nbformat and ipykernel. Unclear …
binste a166ee0
Use Final for constants
binste d34d07d
Mark some type aliases/protocols as private which are new and where w…
binste 30af9a2
Merge branch 'master' into type_hint_vl_v5_files
jonmmease 4ef8f93
Remove unused 'type ignore' statements. Add mypy error if it detects …
binste c6e866e
Merge branch 'master' into type_hint_vl_v5_files
binste 1261511
Replace _SupportsDataFrame with _DataFrameLike
binste fa9c081
Format code
binste 1b0d531
Rename two typeddicts to make it clearer that they reference the retu…
binste 15f89fd
Make InferredVegaLiteType private
binste 54c0f03
Merge branch 'master' into type_hint_vl_v5_files
binste e974bc9
Type hint 'data' in infer_vegalite_type as object as this is the type…
binste File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question, here you mention:
data: Union[np.ndarray, pd.Series]
. When I look to where this is used: https://github.com/altair-viz/altair/blob/master/altair/vegalite/v5/api.py#L2411, I cannot understand why we need to be restrictive on the input types from numpy and pandas here. I really like types likeDataFrameLike
andSupportsGeoInterface
for input data features (so we can be flexible on input and restrictive on output).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to define a type hint named
ArrayLike1D
something that can serve for one-dimensional data of various forms, but not sure if this is possible yet (eg. python/mypy#12280).Otherwise, maybe something like they define as type hint for the
values
of a polars series? There they define anArrayLike
type as such (see here:And in the docstring for values of the series they mention:
I don't think this type-hint is strictly 1D as the docstring suggest, but it can serve as a reference.
Somehow wished it would be possible to include
range
as well (ref: #2877).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that's too restrictive. I used the type hints which were already present in the docstring but seems like this function supports anything that the pandas function
infer_type
can handle. According to their own type hints this is allobject
so basically everything. I'd suggest that we type hint it the same to be consistent with pandas. Implemented in e974bc9Btw, the function
infer_vegalite_type
is used only here. Your link points to the usage ofinfer_encoding_types
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is flexible enough, thanks for the change.