fix(typing): Improve `Then` annotations, autocompletion, docs #3567

dangotbanned · 2024-09-04T12:22:24Z

Overview of issues by @binste in comment

The draft PR is very helpful! Trying to summarise/reword it for myself:

Then should not inherit from SchemaBase as it muddies autocompletion
We need a type hint which lets a SchemaBase pass due to alt.condition and alt.when().then().otherwise(). Can be SchemaBase or a protocol
It's unclear to me when, as a developer, I'd use SchemaLike. If I understand it correctly, we mainly introduce it now to make these types work.
Purpose:
- Fix type errors users might get right now, keeping types as narrow as possible so they are more useful
- Improving UX: keep/improve autocompletion + provide a good visual indication that we mean conditions can be passed

How about

@runtime_checkable
class Condition(Protocol):
   """We use this protocol to signal to a user that they can pass any condition created by either `alt.condition` or `alt.when`.

   Other SchemaBase instances will pass as `Condition` as well and so for type checkers it won't matter but using this protocol when only conditions are expected at least gives a better visual indication to users.
   """
    _schema: ClassVar[_SchemaLikeDict] = {"type": "object"}

    def to_dict(self, *args, **kwds) -> Any: ...


def chart_encode(
   col_7: Optional[str | Color | Condition | Map | ColorDatum | ColorValue] = Undefined,
)

Condition is not yet used anywhere in the codebase and it makes it clearer what we intend to do with the protocol. What do you think?

Tasks

Allow Then in Chart.encode(), without subclassing SchemaBase
- See comment for screenshots of old autocomplete
Experiment with simplifying Chart.encode() annotations
- comment
- 08bf16e (#3567)
Standardise user-facing (but non-public) aliases & documentation
- (Condition|Schema)Like
  - d20109e (#3567)
- Various TypeAlias|TypedDict in api.py
  - 0a6d599 (#3567)
  - 0a93309 (#3567)

Note

This PR started as a draft proposal for addressing multiple related issues.
See #3552 (comment) for further info

Draft PR Demo

Purely demonstrating type checking behavior for #3552 (comment)

Extended chart_encode

Note

mypy failing is expected and part of the demonstration

Purely demonstrating type checking behaviour for vega#3552 (comment)

Revisting these I found it confusing that they were defined like (A, B, B, A). They are now (A, A, B, B) so it is clearer how they are linked

Aiming to get all of these to pass, without breaking anything else. **CI fail expected**

…-compliant Can't define this in `OperatorMixin`, as `Expression` uses `{"type": "string"}`

Added a lot of notes here, since I was surpised `Then` would not be allowed in the cases I had planned originally

We're using a feature that didn't make it into `3.13` yet https://peps.python.org/pep-0728/

The warning raised by `pyright` here was actually quite helpful. Essentially, a shorthand string is never allowed here vega#3552 (comment)

We now provide more precise errors identifying exactly which call produced the issue. Previously, the `.then("min(foo):Q")` was silently allowed - but later rejected in `.otherwise()`.

I'm fairly sure this is fixed in another branch, so leaving this here for less of a conflict https://github.com/vega/altair/pull/3547/files#diff-bbe4e187b18d242a366c820d023afd89041759cb96e4ec66c3f34559a72c2f9d

No longer inherited

altair/vegalite/v5/api.py

tests/vegalite/v5/test_api.py

altair/vegalite/v5/api.py

altair/vegalite/v5/schema/channels.py

altair/utils/schemapi.py

vega#3567 (comment)

Removes the need for hardcoding restrictions in multiple places

…edDict`) - Adds some more consistency between `condition` and `when-then-otherwise` - 1 less thing to think about

This isn't a runtime concern, just to satisfy typing

Mostly pushing this as an experiment. Shortens signatures, but there is a cost to the indirection vega#3567 (comment)

tests/vegalite/v5/test_api.py

binste

LGTM, thanks!

altair/vegalite/v5/api.py

dangotbanned · 2024-09-22T15:11:34Z

LGTM, thanks!

Thanks for the review @binste

dangotbanned added 2 commits September 4, 2024 13:21

fix(DRAFT): Add examples for SchemaLike

331a52e

Purely demonstrating type checking behaviour for vega#3552 (comment)

chore(ruff): Ignore unused variables

633c0c2

dangotbanned mentioned this pull request Sep 4, 2024

_EncodingMixin.encode types defined too narrow #3552

Closed

dangotbanned added 4 commits September 14, 2024 12:02

Merge remote-tracking branch 'upstream/main' into schema-like-proto

f59a290

style: Reorder _Condition... definitions

f34abe9

Revisting these I found it confusing that they were defined like (A, B, B, A). They are now (A, A, B, B) so it is clearer how they are linked

chore: Remove example code from test_api

2dc1f21

test: Extend test_when_typing

c86a107

Aiming to get all of these to pass, without breaking anything else. **CI fail expected**

dangotbanned linked an issue Sep 14, 2024 that may be closed by this pull request

_EncodingMixin.encode types defined too narrow #3552

Closed

dangotbanned changed the title ~~fix(DRAFT): Add examples for SchemaLike~~ fix(DRAFT): Improve Then annotations, simplify autocompletion Sep 15, 2024

dangotbanned added the maintenance label Sep 15, 2024

dangotbanned added 17 commits September 15, 2024 14:28

feat(typing): Adds SchemaLike, ConditionLike protocols

5ab0616

refactor: Redefine Then as a ConditionLike

adf3e56

feat: Convert SchemaLike in Chart.encode

7315582

feat(typing): Add IntoCondition alias

d169a1b

feat(typing): Use IntoCondition in Chart.encode annotations

c5632f4

fix: Add more exclusions to generate_api_docs

2f74e8e

build: run generate-schema-wrapper

160b16c

fix: Make Parameter, (Parameter|Selection)Expression SchemaLike…

e470a98

…-compliant Can't define this in `OperatorMixin`, as `Expression` uses `{"type": "string"}`

test: Update test_when_typing

f49c9f5

Added a lot of notes here, since I was surpised `Then` would not be allowed in the cases I had planned originally

chore: Remove now-fixed type ignore

a773c97

fix: Update TypedDict version bound

9f49ef6

We're using a feature that didn't make it into `3.13` yet https://peps.python.org/pep-0728/

test: Remove proposal demo

c420a1c

fix: Confirm ChainedWhen.then structural soundness

7925d90

The warning raised by `pyright` here was actually quite helpful. Essentially, a shorthand string is never allowed here vega#3552 (comment)

test: Update test_when_multiple_fields

960e134

We now provide more precise errors identifying exactly which call produced the issue. Previously, the `.then("min(foo):Q")` was silently allowed - but later rejected in `.otherwise()`.

chore(typing): Add a temporary ignore

5ee39c2

I'm fairly sure this is fixed in another branch, so leaving this here for less of a conflict https://github.com/vega/altair/pull/3547/files#diff-bbe4e187b18d242a366c820d023afd89041759cb96e4ec66c3f34559a72c2f9d

docs: Add summary for _is_condition_closed

969e0e3

feat: Add repr for Then

8cad6af

No longer inherited