refactor(typing): reduce type ignores in `api.py` #3480

dangotbanned · 2024-07-15T18:17:16Z

This PR is mainly working through type: ignore / TODO comments and fixing what I can.

My primary motivation for this was to be able to use pyright within altair more often.

Despite the existing type: ignore comments, I was getting 46 errors in api alone.

Anywhere that it appears I've made a change unrelated to existing comments, please know it was to solve one of these:

Large screenshot

Direct copy from another PR, but is helpful here https://github.com/dangotbanned/altair/blob/d607c70824860ef3478d7d3996d26dc516d69369/altair/vegalite/v5/api.py#L504

tools/schemapi/schemapi.py

pyproject.toml

altair/utils/schemapi.py

altair/vegalite/v5/api.py

binste

Great improvements! I especially like the type intersection :) Added some questions.

altair/utils/schemapi.py

pyproject.toml

tools/schemapi/schemapi.py

altair/vegalite/v5/api.py

dangotbanned · 2024-07-17T11:33:46Z

Great improvements! I especially like the type intersection :) Added some questions.

Thanks for the review @binste, I've just finished going through your comments.

No rush, but I'll wait for your response before reverting/making those changes.

FYI #3480 (commits) is not addressing that

I removed these in 6adf564 due to how complex they were getting, but both `mypy` and `pyright` seem happy with this solution

Following investigation during vega#3480 (comment)

Fixes: - vega#3480 (comment) - vega#3480 (comment) - vega#3480 (comment)

dangotbanned · 2024-07-19T12:25:13Z

@binste as #3480 (comment) was the last part to resolve, I thought I'd go ahead with it in 21a3e84 (#3480)

Hopefully that will make this easier to close, as you can just revert that commit if needed

binste

Thanks again! :)

Wasn't picked up in vega#3480

* docs: adapt `empty` note for `when`, `Then.when` * docs: Add example for `Then.when` * docs: Add an example to `Then.otherwise` * chore(typing): add pyright ignore Wasn't picked up in #3480 * docs: Add example for `When.then` Adapted from the final example in https://vega.github.io/vega-lite/docs/condition.html * docs: Add example for `ChainedWhen.then` * docs: Add real-world examples for `alt.when` Covers everything apart from chained when * docs: Update `polars.when` references Now all link to docs, rather than source code #3492 (comment) * feat: Add `__repr__` for `When`, `ChainedWhen` #3492 (comment) * Update altair/vegalite/v5/api.py Co-authored-by: Mattijn van Hoek <mattijn@gmail.com> --------- Co-authored-by: Mattijn van Hoek <mattijn@gmail.com>

* ci(ruff): Add `ANN` rules for `api.py` only To highlight all the missing annotations to fix and autofix `None` return * feat(typing): Complete annotations for most `api` functions Excluding `*args` on `ChartType` wrappers. They need to be defined in alignment in multiple places, which is more complex * feat(typing): Annotate more expr/params in `api` * feat(typing): Various changes to enforce `dict[str, Any]` instead of `dict` Among these, many locations already assume `str` keys in the implementation - without checking * feat(typing): Misc minor method annotations * feat(typing): Use `ChartType` in all `ChartType` dunder methods * fix(ruff): Ignore some `ANN` rules that won't be fixed * chore: add pyright ignore from #3492 * feat(typing): Improve `ChartType` constructor/factory annotations * feat(typing): Annotate remaining functions in `api` * feat(typing): Complete `RepeatChart` annotations * fix(typing): Resolve Liskov violations ``` altair\vegalite\v5\api.py:4368: error: Argument 1 of "__iadd__" is incompatible with "__add__" of supertype "TopLevelMixin"; supertype defines the argument type as "Chart | RepeatChart | ConcatChart | HConcatChart | VConcatChart | FacetChart | LayerChart" [override] def __iadd__(self, other: LayerChart | Chart) -> Self: ^~~~~~~~~~~~~~~~~~~~~~~~~ altair\vegalite\v5\api.py:4368: note: This violates the Liskov substitution principle altair\vegalite\v5\api.py:4368: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides altair\vegalite\v5\api.py:4376: error: Argument 1 of "__add__" is incompatible with supertype "TopLevelMixin"; supertype defines the argument type as "Chart | RepeatChart | ConcatChart | HConcatChart | VConcatChart | FacetChart | LayerChart" [override] def __add__(self, other: LayerChart | Chart) -> Self: ^~~~~~~~~~~~~~~~~~~~~~~~~ altair\vegalite\v5\api.py:4376: note: This violates the Liskov substitution principle altair\vegalite\v5\api.py:4376: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides ``` * chore(typing): Update more `dict` -> `dict[str, Any]` * style(ruff): fix whitespace * chore: Remove TODO fixed in #3480 * fix(typing): Enable `ANN003` and fix all in `api` * fix(typing): Enable `ANN002` and fix all in `api` * chore(ruff): Add note on `ANN` This could later be extended to other modules, but for now `api` is complete. * fix(typing): Add missing `FacetChart` annotations To align with the other `ChartType`s

…copy)` The call to `.to_dict`, and 3 line comment also appeared in the traceback for validation errors on `ChartType`s. This removes the need for the comment and type ignore that it is describing. See vega#3480 for more info on the introduction of `_top_schema_base`

dangotbanned added 8 commits July 15, 2024 19:14

refactor(typing): reduce type ignores in api.py

6adf564

Merge branch 'main' into reduce-api-type-ignore

1fb6f1f

Merge branch 'main' into reduce-api-type-ignore

6473ef3

feat(typing): adds _is_undefined guard

426336c

Direct copy from another PR, but is helpful here https://github.com/dangotbanned/altair/blob/d607c70824860ef3478d7d3996d26dc516d69369/altair/vegalite/v5/api.py#L504

fix(typing): ignore[index] from vega#3455

32e4fc3

fix(typing): [ignore[arg-type] from vega#3452

3ce7077

refactor(typing): use _is_undefined in some cases

222f953

refactor: reduce code duplication in Chart.to_dict

419a4e9