From 53cf007de0fbcd8aee5e49b6585996187f421ab1 Mon Sep 17 00:00:00 2001 From: Joel Ostblom Date: Sun, 5 Apr 2026 10:46:36 +0200 Subject: [PATCH 1/3] Apply fixes from Codex review What I corrected/simplified: - In FieldChannelMixin.to_dict (altair/vegalite/v6/schema/channels.py:163), I moved expression/variable-param handling before parsed-shorthand context setup. - This removes the need for the previous context.pop("parsed_shorthand", None) workaround and avoids context leakage by design. - Behavior is unchanged for normal field-string paths, but this is cleaner and less fragile. What I added to tests: - Added a missing regression test for the bare channel case: - encode(x=param) (not just encode(x=alt.X(param, ...))) - File: tests/vegalite/v6/test_api.py:2259 - This ensures _wrap_in_channel path for variable params is covered. --- altair/vegalite/v6/schema/channels.py | 19 +++++++++++----- tests/vegalite/v6/test_api.py | 31 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/altair/vegalite/v6/schema/channels.py b/altair/vegalite/v6/schema/channels.py index d2feb8695f..842b7d26e9 100644 --- a/altair/vegalite/v6/schema/channels.py +++ b/altair/vegalite/v6/schema/channels.py @@ -15,7 +15,7 @@ import narwhals.stable.v1 as nw -from altair.expr.core import Expression +from altair.expr.core import Expression, GetItemExpression from altair.utils import infer_encoding_types as _infer_encoding_types from altair.utils import parse_shorthand from altair.utils.schemapi import Undefined, _infer_expr_type, with_property_setters @@ -186,12 +186,21 @@ def to_dict( # noqa: C901 for sh in shorthand ] - shorthand_or_kwds = shorthand + from altair.vegalite.v6.api import Parameter + + def _param_to_expr(val: Any) -> Any: + """Convert a VariableParameter to a datum[param] expression.""" + if isinstance(val, Parameter) and val.param_type == "variable": + return GetItemExpression("datum", val) + return val + + shorthand_or_kwds = _param_to_expr(shorthand) if shorthand_or_kwds is Undefined: - # Look for an Expression or ExprRef in the channel kwds. + # Look for an Expression, ExprRef, or VariableParameter in channel kwds. for val in self._kwds.values(): # type: ignore[attr-defined] - if isinstance(val, (Expression, core.ExprRef)): - shorthand_or_kwds = val + converted = _param_to_expr(val) + if isinstance(converted, (Expression, core.ExprRef)): + shorthand_or_kwds = converted break if isinstance(shorthand_or_kwds, (Expression, core.ExprRef)): diff --git a/tests/vegalite/v6/test_api.py b/tests/vegalite/v6/test_api.py index 958877e675..70333cfe00 100644 --- a/tests/vegalite/v6/test_api.py +++ b/tests/vegalite/v6/test_api.py @@ -2240,6 +2240,37 @@ def test_inline_calc_deduplication(): assert spec["encoding"]["y"]["field"] == field_name +def test_inline_calc_variable_parameter_as_shorthand(): + """A VariableParameter used as encoding value expands to datum[param_name].""" + param = alt.param(name="xcol", value="x") + chart = ( + alt.Chart({"values": [{"x": 1}]}) + .mark_point() + .encode(x=alt.X(param, type="quantitative")) + .add_params(param) + ) + spec = chart.to_dict() + transforms = spec.get("transform", []) + assert len(transforms) == 1 + assert transforms[0]["calculate"] == "datum[xcol]" + field_name = transforms[0]["as"] + assert spec["encoding"]["x"]["field"] == field_name + + +def test_inline_calc_variable_parameter_bare_channel_value(): + """A VariableParameter used directly as x=param also expands to datum[param_name].""" + param = alt.param(name="xcol", value="x") + chart = ( + alt.Chart({"values": [{"x": 1}]}).mark_point().encode(x=param).add_params(param) + ) + spec = chart.to_dict() + transforms = spec.get("transform", []) + assert len(transforms) == 1 + assert transforms[0]["calculate"] == "datum[xcol]" + field_name = transforms[0]["as"] + assert spec["encoding"]["x"]["field"] == field_name + + def test_inline_calc_explicit_type_overrides_inferred(): """An explicit type on the channel overrides type inference.""" expr = alt.datum.x + alt.datum.y # would infer "quantitative" From b32a8cf76ce56e707dafd41c4d1b84d9ee9b9c3c Mon Sep 17 00:00:00 2001 From: Joel Ostblom Date: Sun, 5 Apr 2026 14:26:11 +0200 Subject: [PATCH 2/3] Support parameters inline in encoding fields --- altair/expr/core.py | 2 +- altair/utils/core.py | 14 ++++++++++++-- altair/vegalite/v6/schema/channels.py | 2 +- doc/user_guide/api.rst | 1 + tests/expr/test_expr.py | 7 +++++++ tools/generate_schema_wrapper.py | 19 ++++++++++++++----- 6 files changed, 36 insertions(+), 9 deletions(-) diff --git a/altair/expr/core.py b/altair/expr/core.py index eff6814892..4fb927e602 100644 --- a/altair/expr/core.py +++ b/altair/expr/core.py @@ -281,7 +281,7 @@ def __init__(self, group, name) -> None: super().__init__(group=group, name=name) def __repr__(self) -> str: - return f"{self.group}[{self.name!r}]" + return f"{self.group}[{_js_repr(self.name)}]" IntoExpression: TypeAlias = Union[ diff --git a/altair/utils/core.py b/altair/utils/core.py index fe2b712091..a7cb17eef5 100644 --- a/altair/utils/core.py +++ b/altair/utils/core.py @@ -895,8 +895,9 @@ def get_encoding(self, tp: type[Any], /) -> str: msg = f"positional of type {type(tp).__name__!r}" raise NotImplementedError(msg) - def _wrap_in_channel(self, obj: Any, encoding: str, /): - from altair.expr.core import Expression + def _wrap_in_channel(self, obj: Any, encoding: str, /): # noqa: C901 + from altair.expr.core import Expression, GetItemExpression + from altair.vegalite.v6.api import Parameter from altair.vegalite.v6.schema.core import ExprRef if isinstance(obj, (Expression, ExprRef)): @@ -907,6 +908,15 @@ def _wrap_in_channel(self, obj: Any, encoding: str, /): return tp(shorthand=obj) return obj + # A VariableParameter used directly as an encoding value is sugar for + # alt.datum[param], which generates a transform_calculate: datum[param_name]. + if isinstance(obj, Parameter) and obj.param_type == "variable": + expr = GetItemExpression("datum", obj) + if channel := self.name_to_channel.get(encoding): + tp = channel["field"] + return tp(shorthand=expr) + return obj + if isinstance(obj, SchemaBase): return obj elif isinstance(obj, str): diff --git a/altair/vegalite/v6/schema/channels.py b/altair/vegalite/v6/schema/channels.py index 842b7d26e9..211de3ec0d 100644 --- a/altair/vegalite/v6/schema/channels.py +++ b/altair/vegalite/v6/schema/channels.py @@ -189,7 +189,7 @@ def to_dict( # noqa: C901 from altair.vegalite.v6.api import Parameter def _param_to_expr(val: Any) -> Any: - """Convert a VariableParameter to a datum[param] expression.""" + # Convert a VariableParameter to a datum[param] expression. if isinstance(val, Parameter) and val.param_type == "variable": return GetItemExpression("datum", val) return val diff --git a/doc/user_guide/api.rst b/doc/user_guide/api.rst index 79cabd7226..fbc0b13223 100644 --- a/doc/user_guide/api.rst +++ b/doc/user_guide/api.rst @@ -56,6 +56,7 @@ Encoding Channels FillOpacityDatum FillOpacityValue FillValue + GetItemExpression Href HrefValue Key diff --git a/tests/expr/test_expr.py b/tests/expr/test_expr.py index addc29477e..6565daf897 100644 --- a/tests/expr/test_expr.py +++ b/tests/expr/test_expr.py @@ -10,6 +10,7 @@ import pytest from jsonschema.exceptions import ValidationError +import altair as alt from altair import datum, expr, ExprRef from altair.expr import _ExprMeta from altair.expr.core import Expression, GetAttrExpression @@ -167,6 +168,12 @@ def test_expression_getitem(): assert repr(x) == "datum.foo[0]" +def test_datum_getitem_param(): + xcol = alt.param(name="xcol", value="Horsepower") + x = datum[xcol] + assert repr(x) == "datum[xcol]" + + def test_expression_function_expr(): # test including an expr. should return an ExprRef er = expr(expr.PI * 2) diff --git a/tools/generate_schema_wrapper.py b/tools/generate_schema_wrapper.py index 4a00c9cc18..0869159521 100644 --- a/tools/generate_schema_wrapper.py +++ b/tools/generate_schema_wrapper.py @@ -132,12 +132,21 @@ def to_dict( # noqa: C901 for sh in shorthand ] - shorthand_or_kwds = shorthand + from altair.vegalite.v6.api import Parameter + + def _param_to_expr(val: Any) -> Any: + # Convert a VariableParameter to a datum[param] expression. + if isinstance(val, Parameter) and val.param_type == "variable": + return GetItemExpression("datum", val) + return val + + shorthand_or_kwds = _param_to_expr(shorthand) if shorthand_or_kwds is Undefined: - # Look for an Expression or ExprRef in the channel kwds. + # Look for an Expression, ExprRef, or VariableParameter in channel kwds. for val in self._kwds.values(): # type: ignore[attr-defined] - if isinstance(val, (Expression, core.ExprRef)): - shorthand_or_kwds = val + converted = _param_to_expr(val) + if isinstance(converted, (Expression, core.ExprRef)): + shorthand_or_kwds = converted break if isinstance(shorthand_or_kwds, (Expression, core.ExprRef)): @@ -911,7 +920,7 @@ def generate_vegalite_channel_wrappers(fp: Path, /) -> ModuleDef[list[str]]: "import sys", "from typing import Any, overload, Literal, Union, TypedDict", "import narwhals.stable.v1 as nw", - "from altair.expr.core import Expression", + "from altair.expr.core import Expression, GetItemExpression", "from altair.utils import infer_encoding_types as _infer_encoding_types", "from altair.utils import parse_shorthand", "from altair.utils.schemapi import Undefined, _infer_expr_type, with_property_setters", From 52fe728093623b090987ab1e6f7f523a9161f632 Mon Sep 17 00:00:00 2001 From: Joel Ostblom Date: Sun, 5 Apr 2026 14:31:03 +0200 Subject: [PATCH 3/3] Don't overwrite explicit titles --- altair/vegalite/v6/schema/channels.py | 5 +++++ tests/vegalite/v6/test_api.py | 14 ++++++++++++++ tools/generate_schema_wrapper.py | 5 +++++ 3 files changed, 24 insertions(+) diff --git a/altair/vegalite/v6/schema/channels.py b/altair/vegalite/v6/schema/channels.py index 211de3ec0d..c47c403a55 100644 --- a/altair/vegalite/v6/schema/channels.py +++ b/altair/vegalite/v6/schema/channels.py @@ -222,6 +222,11 @@ def _param_to_expr(val: Any) -> Any: result["type"] = explicit_type elif inferred := _infer_expr_type(shorthand_or_kwds): result["type"] = inferred + + explicit_title = self._get("title") # type: ignore[attr-defined] + if explicit_title is not Undefined: + result["title"] = explicit_title + return result if shorthand is Undefined: diff --git a/tests/vegalite/v6/test_api.py b/tests/vegalite/v6/test_api.py index 70333cfe00..be98048ee8 100644 --- a/tests/vegalite/v6/test_api.py +++ b/tests/vegalite/v6/test_api.py @@ -2304,3 +2304,17 @@ def test_inline_calc_exprref_string_syntax(): field_name = transforms[0]["as"] assert spec["encoding"]["x"]["field"] == field_name assert spec["encoding"]["x"]["type"] == "quantitative" + + +def test_inline_calc_title_none_respected_for_variable_parameter(): + """Explicit title(None) should be preserved for inline variable-parameter channels.""" + xcol_param = alt.param(name="xcol", value="x") + chart = ( + alt.Chart(pd.DataFrame({"x": [1, 2, 3]})) + .mark_point() + .encode(x=alt.X(xcol_param).type("quantitative").title(None)) + .add_params(xcol_param) + ) + + spec = chart.to_dict() + assert spec["encoding"]["x"]["title"] is None diff --git a/tools/generate_schema_wrapper.py b/tools/generate_schema_wrapper.py index 0869159521..9f3ebac281 100644 --- a/tools/generate_schema_wrapper.py +++ b/tools/generate_schema_wrapper.py @@ -168,6 +168,11 @@ def _param_to_expr(val: Any) -> Any: result["type"] = explicit_type elif inferred := _infer_expr_type(shorthand_or_kwds): result["type"] = inferred + + explicit_title = self._get("title") # type: ignore[attr-defined] + if explicit_title is not Undefined: + result["title"] = explicit_title + return result if shorthand is Undefined: