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): Improve Then annotations, autocompletion, docs #3567

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Sep 4, 2024

Fixes #3552

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

Related

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

image

image

Note

mypy failing is expected and part of the demonstration

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**
@dangotbanned dangotbanned linked an issue Sep 14, 2024 that may be closed by this pull request
@dangotbanned dangotbanned changed the title fix(DRAFT): Add examples for SchemaLike fix(DRAFT): Improve Then annotations, simplify autocompletion Sep 15, 2024
…-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()`.
No longer inherited
Comment on lines +1087 to +1096
def __repr__(self) -> str:
name = type(self).__name__
COND = "condition: "
LB, RB = "{", "}"
if len(self.condition) == 1:
args = f"{COND}{self.condition!r}".replace("\n", "\n ")
else:
conds = "\n ".join(f"{c!r}" for c in self.condition)
args = f"{COND}[\n " f"{conds}\n ]"
return f"{name}({LB}\n {args}\n{RB})"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, this is the short/long form

image

Comment on lines +685 to +691
with pytest.raises(SchemaValidationError):
# NOTE: `when-then` is allowed as an encoding, but not as a `ConditionalAxisProperty`
# The latter fails validation since it does not have a default `value`
chart.encode(
color=then,
x=alt.X("Cylinders:N").axis(labelColor=then), # type: ignore[call-overload]
y=alt.Y("Origin:N", axis=alt.Axis(labelColor=then)), # type: ignore[arg-type]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised that the current schema doesn't allow this.

Conditional encodings can omit the default, but conditional properties must define one.
I find this strange as all the conditional properties I tested accept value=None - so maybe this support could be added in vega-lite?

Comment on lines 25004 to 25030
angle: Optional[
str | Angle | Map | AngleDatum | AngleValue | IntoCondition
] = Undefined,
color: Optional[
str | Color | Map | ColorDatum | ColorValue | IntoCondition
] = Undefined,
column: Optional[str | Column | Map | IntoCondition] = Undefined,
description: Optional[
str | Description | Map | DescriptionValue | IntoCondition
] = Undefined,
detail: Optional[OneOrSeq[str | Detail | Map | IntoCondition]] = Undefined,
facet: Optional[str | Facet | Map | IntoCondition] = Undefined,
fill: Optional[
str | Fill | Map | FillDatum | FillValue | IntoCondition
] = Undefined,
fillOpacity: Optional[
str | FillOpacity | Map | FillOpacityDatum | FillOpacityValue
str
| FillOpacity
| Map
| FillOpacityDatum
| FillOpacityValue
| IntoCondition
] = Undefined,
href: Optional[str | Href | Map | HrefValue | IntoCondition] = Undefined,
key: Optional[str | Key | Map | IntoCondition] = Undefined,
latitude: Optional[
str | Latitude | Map | LatitudeDatum | IntoCondition
Copy link
Member Author

@dangotbanned dangotbanned Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we could simplify these by adding some intermediate, non-exported aliases.

So for the first two channels:

Imports/defs
from typing import Union
from typing_extensions import TypeAlias

from altair import (
    Angle,
    AngleDatum,
    AngleValue,
    Color,
    ColorDatum,
    ColorValue,
    Undefined,
)
from altair.utils import Optional
from altair.vegalite.v5.api import IntoCondition
from altair.vegalite.v5.schema._typing import Map

AnyAngle: TypeAlias = Union[Angle, AngleDatum, AngleValue]
AnyColor: TypeAlias = Union[Color, ColorDatum, ColorValue]
class _EncodingMixin:
    def encode_current(
        self,
        *args: Any,
        angle: Optional[
            str | Angle | Map | AngleDatum | AngleValue | IntoCondition
        ] = Undefined,
        color: Optional[
            str | Color | Map | ColorDatum | ColorValue | IntoCondition
        ] = Undefined,
    ): ...
    def encode_proposed(
        self,
        *args: Any,
        angle: Optional[str | AnyAngle | Map | IntoCondition] = Undefined,
        color: Optional[str | AnyColor | Map | IntoCondition] = Undefined,
    ): ...

Would certainly reduce the amount of repetition in all the names.
Since there are 40 channels, with up to 3 names each - adding just a single new type IntoCondition has a big impact

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this thread, regarding potential trade-off with an alias here

IMO this is isn't as big of an issue as before - but still relevant

@dangotbanned dangotbanned changed the title fix(DRAFT): Improve Then annotations, simplify autocompletion fix(typing): Improve Then annotations, autocompletion, docs Sep 16, 2024
@dangotbanned dangotbanned marked this pull request as ready for review September 16, 2024 15:15
@@ -730,7 +767,7 @@ def test_when_then_interactive() -> None:
.encode(
x="IMDB_Rating:Q",
y="Rotten_Tomatoes_Rating:Q",
color=alt.when(predicate).then(alt.value("grey")), # type: ignore[arg-type]
color=alt.when(predicate).then(alt.value("grey")),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not requiring this # type: ignore demonstrates that #3552 is fixed

@dangotbanned dangotbanned enabled auto-merge (squash) September 17, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_EncodingMixin.encode types defined too narrow
1 participant