fix: cleanup ruff rules

[alec-bike]

This PR tidies up some ruff rules and comments in pyproject.toml:

• increase max-complexity to 15 (allows removal of some noqa: C901 annotations) reverted
• remove commented-out preview-only ruff rules¹

Also, the experimental tool.pyright section has been simplified (and tested against pyright and basedpyright).

Footnotes

1. ruff preview was turned off in fix: type issues with Chart mark methods. #3936 ↩

[dangotbanned]

@alec-bike there are 2 PRs here

Pyright

Also, the experimental tool.pyright section has been simplified (and tested against pyright and basedpyright).

Could you set up pyright to run in CI first, before making changes like this please?

I was working towards that a while back, but there's nothing verifying your claims.

Locally, pyright is complaining about things that are invisible to a reviewer:

Show arbitrary example

Ruff

I don't see any value in removing C901 comments.

Refactoring the functions is a good way to learn about how altair works or just simplify things for future readers 🙂

[alec-bike]

Thanks for the review @dangotbanned.

Could you set up pyright to run in CI first, before making changes like this please?

I've been wondering about the status of pyright (and multiple type checkers) in altair. Is the goal for altair to support both mypy and pyright in CI?

I've been running ty for my testing, using a local ty.toml config to keep it separate from pyproject.toml. Would altair also want to run ty in CI?

[dangotbanned]

#3967 (comment)

I've been wondering about the status of pyright (and multiple type checkers) in altair. Is the goal for altair to support both mypy and pyright in CI?

I guess this is a history of the progress I made with pyright support:

• https://github.com/vega/altair/pulls?q=sort%3Aupdated-desc+is%3Apr+author%3Adangotbanned+is%3Amerged+typing

Before that, there was a long road to supporting mypy:

• https://github.com/vega/altair/pulls?q=sort%3Aupdated-desc+is%3Apr+mypy+is%3Amerged+author%3Abinste

This section of the config was where there were still gaps, but IIRC some test modules were an issue as well:

altair/pyproject.toml

Lines 374 to 378 in 356c78b

	ignore = [
	"./altair/vegalite/v6/schema/channels.py", # 716 warns
	"./altair/vegalite/v6/schema/mixins.py", # 1001 warns
	"./altair/jupyter/", # Mostly untyped
	"./tests/test_jupyter_chart.py", # Based on untyped module

I've been running ty for my testing, using a local ty.toml config to keep it separate from pyproject.toml. Would altair also want to run ty in CI?

I'm very much looking forward to using ty when it is ready, but there are many gaps that impact an upstream project I contribute to:

• Tracking: Adopt ty as an (additional) type checker narwhals-dev/narwhals#3409

Using ty locally is probably the best move for now - I have no doubt it will play a really important role in moving python forward.

Running pyright in ci is quite doable - with the right amount of effort.
If you look at Python Type System Conformance Test Results - it really is the bar for following the rules 😄
Wish it was faster though

[dangotbanned]

Thanks for the changes @alec-bike

I do still have a few more questions though

[dangotbanned]

Suggested change

	kwds: dict = {
	kwds: Map = {

That alias expands to a type that actually supports **kwds

altair/altair/vegalite/v6/schema/_typing.py

Line 224 in 356c78b

Map: TypeAlias = Mapping[str, Any]

from __future__ import annotations

from typing import Any


def unpack(**kwargs: Any) -> dict[str, Any]:
    return kwargs


def any_dict(kwds: dict) -> dict[str, Any]:
    return unpack(**kwds)


def safe_dict(kwds: dict[str, Any]) -> dict[str, Any]:
    return unpack(**kwds)


any_dict({"keyword": "argument"})
any_dict({1: "boom"})

safe_dict({"keyword": "argument"})
safe_dict({1: "boom"})
# Argument of type "dict[int, str]" cannot be assigned to parameter "kwds" of type
# "dict[str, Any]" in function "safe_dict"
#  "Literal[1]" is not assignable to "str"

Traceback (most recent call last):
  File "../t.py", line 19, in <module>
    any_dict({1: "boom"})
    ~~~~~~~~^^^^^^^^^^^^^
  File "../t", line 11, in any_dict
    return unpack(**kwds)
TypeError: keywords must be strings

[alec-bike]

Interesting. ty flagged kwds: Map as invalid-argument-type whereas kwds: dict fixed the ty warning. I've reverted this for now and will check with the ty devs.

[dangotbanned]

The Any is a result of the change made on _top_schema_base

Suggested change

	vegalite_spec: Any = _top_schema_base(super(TopLevelMixin, copy)).to_dict(
	vegalite_spec = _top_schema_base(super(TopLevelMixin, copy)).to_dict(

[alec-bike]

I added this today to revert a change I'd made in commit a2afad3. That commit fixed a ty issue but introduced a pyright error.

I also reverted line 460 back to nm: Any.

With these changes, I no longer see pyright errors on those lines.