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

Potentially simplify _check_if_valid_subspec and _check_if_can_be_layered #3518

Closed
binste opened this issue Aug 4, 2024 · 1 comment · Fixed by #3520
Closed

Potentially simplify _check_if_valid_subspec and _check_if_can_be_layered #3518

binste opened this issue Aug 4, 2024 · 1 comment · Fixed by #3520

Comments

@binste
Copy link
Contributor

binste commented Aug 4, 2024

What is your suggestion?

See #3508 (comment). CC @dangotbanned

Have you considered any alternative solutions?

No response

@dangotbanned
Copy link
Member

dangotbanned commented Aug 4, 2024

Thanks for the ping @binste

More detail in the linked comment, but this would be looking at removing some dict-related code (+ maybe annotations) for compound charts.

Constructing charts in that way appears to always error, since only the earlier parts assume a dict may have been provided by the user.

IIRC there wasn't test coverage for those scenarios, nor was it demonstrated in examples

dangotbanned added a commit to dangotbanned/altair that referenced this issue Aug 4, 2024
binste pushed a commit that referenced this issue Aug 11, 2024
* refactor: Simplify compound chart checks

#3518

* test: Simplify associated tests

- https://docs.python.org/3/library/exceptions.html#TypeError
https://docs.pytest.org/en/latest/reference/reference.html#pytest-raises
https://docs.astral.sh/ruff/rules/pytest-raises-too-broad/

* ci(typing): Disable `pyright.reportUnusedExpression`

Conflicts with most of the test suite and all examples. `ruff` handles this anyway

* refactor: Remove redundant `SchemaBase` check

Reaching `_check_if_can_be_layered` requires passing `_check_if_valid_subspec` - which already raises if not `SchemaBase`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants