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

Do we need to validate the method-based attribute setting separately? #2805

Open
joelostblom opened this issue Jan 4, 2023 · 2 comments
Open
Labels

Comments

@joelostblom
Copy link
Contributor

In #1629, Jake introduced a comment about validating the method-based attribute setting with the schema:

https://github.com/altair-viz/altair/blob/88b706f4afbef00df7c2f724d42f0e001846a3b7/tools/schemapi/schemapi.py#L637-L641

I am not sure if we need to do this as it seems sufficient that the spec is already validated via the usual validation approach. Does anyone see a use for a separate validation here or can we safely remove this comment?

@joelostblom joelostblom added the bug label Jan 4, 2023
@mattijn mattijn added question and removed bug labels Jan 4, 2023
@ChristopherDavisUCI
Copy link
Contributor

My understanding (and I could be way off) when I saw this was that it was just to have more useful error messages. For example, I tried the following (where it should instead be scheme="turbo")

import altair as alt
from vega_datasets import data

cars = data.cars.url

alt.Chart(cars).mark_circle().encode(
    color=alt.Color('Acceleration:Q').scale("turbo")
)

and I believe in the ~100 line error message, the word scale never appears.

So to my understanding, the idea makes sense but definitely is not a prerequisite to a Release Candidate.

@joelostblom
Copy link
Contributor Author

joelostblom commented Jan 6, 2023

The traceback is definitely longer when using .scale vs scale=alt.Scale, but it does end in the same error message for me, which does point the user to what is wrong:

SchemaValidationError: Invalid specification

        altair.vegalite.v5.schema.channels.Color->scale, validating 'anyOf'

        'turbo' is not valid under any of the given schemas

It would be nice with a shorter error message though and it makes sense that validating earlier could shorten it. I wonder if validating an additional time would be affected by #2744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants