Skip to content

refactor(typing): use closed=True in some TypedDicts - #3800

Open
MarcoGorelli wants to merge 3 commits into
narwhals-dev:mainfrom
MarcoGorelli:closed-typeddict
Open

refactor(typing): use closed=True in some TypedDicts#3800
MarcoGorelli wants to merge 3 commits into
narwhals-dev:mainfrom
MarcoGorelli:closed-typeddict

Conversation

@MarcoGorelli

@MarcoGorelli MarcoGorelli commented Jul 17, 2026

Copy link
Copy Markdown
Member

closes #3661

this becomes a bit easier to review if you hide whitespace changes: https://github.com/narwhals-dev/narwhals/pull/3800/changes?w=1

i've unpinned mypy/pyrefly from pyproject.toml, as the version used in CI is taken from the lock file anyway, and like this dependabot updates will take care of those updates. note that we need a mypy version more recent than 2.1.0 for this PR to work

Description

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

  • Related issue #<issue number>
  • Closes #<issue number>

AI assistance

  • No AI tools were used for this PR.
  • AI tools were used.

Checklist

  • Code follows style guide (ruff)

  • Tests added

  • Documented the changes

  • If this is your first PR to narwhals, attach a screenshot of pytest passing locally (not CI):

    PYTEST_ADDOPTS="--numprocesses=logical" \
    make run-ci DEPS="--extra pandas --extra dask --group core-tests --group sklearn --group plugins" \
    CMD="pytest tests --cov=src --cov=tests --runslow --constructors=pandas,pandas[nullable],pandas[pyarrow],pyarrow,polars[eager],polars[lazy],dask,duckdb,sqlframe"

@MarcoGorelli
MarcoGorelli marked this pull request as ready for review July 17, 2026 13:54
@MarcoGorelli

Copy link
Copy Markdown
Member Author

@dangotbanned any objections?

@dangotbanned

Copy link
Copy Markdown
Member

note that we need a mypy version more recent than 2.1.0 for this PR to work

I did figure out a workaround, that still let's you use them at runtime 😅

Show hack

class FieldOpen(HasType, MaybeTitle, TypedDict):
"""Shared by all fields."""
# `HasRepeat` is always allowed, but not sure how it would make sense in narwhals
field: FieldName # | HasRepeat
aggregate: Optional[Aggregate]
timeUnit: NotRequired[TimeUnit]
bin: NotRequired[Bin]
scale: NotRequired[Scale]
class ValueOpen(TypedDict):
"""Shared by all values."""
value: InnerValue
if TYPE_CHECKING:
# https://github.com/python/mypy/pull/21382
class Field(FieldOpen, closed=True): ... # type: ignore[call-arg]
class Value(ValueOpen, closed=True): ... # type: ignore[call-arg]
else:
Field = FieldOpen
Value = ValueOpen
# NOTE: `mypy` stops understanding any of the fields if `closed=True`
# is used with the functional syntax
AggField = TypedDict(
"AggField", {"field": FieldName, "op": AggregateOp, "as": "NotRequired[FieldName]"}
)

Was basically the only way to get ** to work for this mess

return ConditionalField(condition=conditions, **_require_field(last, chart, channel))

class _Conditional(TypedDict):
condition: TestValue | list[TestValue]
if TYPE_CHECKING:
class ConditionalDatum(_Conditional, alt_t.Datum, TypedDict, closed=True): ... # type: ignore[call-arg]
class ConditionalField(_Conditional, alt_t.FieldOpen, TypedDict, closed=True): ... # type: ignore[call-arg]
class ConditionalValue(TypedDict, closed=True): # type: ignore[call-arg]
condition: TestAny | list[TestValue]
value: NotRequired[alt_t.InnerValue]
"""Optional else-clause."""
else:
class ConditionalDatum(_Conditional, alt_t.Datum, TypedDict): ...
class ConditionalField(_Conditional, alt_t.FieldOpen, TypedDict): ...
class ConditionalValue(_Conditional, alt_t.ValueOpen, TypedDict):
condition: TestAny | list[TestValue]
value: NotRequired[alt_t.InnerValue]
"""Optional else-clause."""
# fmt: on

I'm not saying we need this for older mypy, but I believe we would for this part of #3661:

  • Make sure any TYPE_CHECKING tricks play nicely with mkdocstrings

So the situation with that one is if we add (https://mkdocstrings.github.io/griffe/extensions/built-in/unpack-typeddict/)
then the @overloads for from_native should "expand" in the API ref.

I was thinking that might be useful, since we haven't exposed the 348949020 TypedDicts (and I don't plan to 😉) - so unlike in an IDE - you can't tell that easily what parameters are passed.

Griffe doesn't discern closed for the signatures, but I was unsure when I wrote #3661 if TYPE_CHECKING might complicate things?

@dangotbanned

dangotbanned commented Jul 23, 2026

Copy link
Copy Markdown
Member

Another option could be defining all of them in a .pyi file?

typing_extensions should be unconditionally available there.
Ruff would also auto-format them to be super dense

@dangotbanned dangotbanned changed the title use closed=True in some TypeDict cases refactor(typing): use closed=True in some TypedDicts Jul 23, 2026
@MarcoGorelli

MarcoGorelli commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

just tried building, and i don't see any difference in the docs

current:

image

this pr

image

@MarcoGorelli

Copy link
Copy Markdown
Member Author

we're not using https://mkdocstrings.github.io/griffe/extensions/built-in/unpack-typeddict/ , is it to move forwards with this and then, if/when someone adds that, we revisit?

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.

Use typed dicts with closed=True in from_native

2 participants