Parse shorthand when creating the condition 2

[ChristopherDavisUCI]

This is a cleaned up version of #2837

Also updated one test to the new condition handling.

[ChristopherDavisUCI]

Two of the failed tests test_selection_to_dict I think are specific to Vega-Lite v3 and v4 (not v5). (I made a change to the v5 api but not to v3 or v4.) Does anyone see anything specific about the other failed tests?

[binste]

@ChristopherDavisUCI Thanks for looking into this! The work you and @joelostblom put into this got me thinking about the whole deep validation approach in general and if there is not a better way using more of the information that jsonschema returns when validating the whole schema, see #2835 (comment). I'll open a PR soon which explains it in detail but wanted to give you a heads-up that I got it working and it solves this issue and many others we have around error messages.

[joelostblom]

Closed in favor of #2842

[ChristopherDavisUCI]

@joelostblom I think some of the code here could still be worth considering. In the current master branch, the code

c = alt.Chart().mark_point().encode(
    size = alt.condition('true', alt.value(100), alt.value(10))
)

will result in c.encoding.size being equal to

SizeValue({
  condition: SizeValue({
    test: 'true',
    value: 100
  }),
  value: 10
})

That inner SizeValue does not seem correct (because it wraps something which is not a valid SizeValue).

Also, if we evaluate c.to_json(validate="deep") on the above chart, an error is still raised, even though I think the chart itself is fine.

Would it be worth me opening a new PR with some of this same code? Or do you see any reason this inner wrapping is desirable?

@binste I'm also happy to hear your thoughts!

[joelostblom]

That inner SizeValue does not seem correct (because it wraps something which is not a valid SizeValue).

I agree with you that this looks odd, and the change you suggested looks more appropriate (although both to_json and to_dict still works fine). I tracked the introduction of the behavior you removed to this PR #1597. Could you investigate that closer and see if there was any specific intention with introducing that double condition wrapping that we risk losing here?

I will reopen this for now so that we can continue the discussion and see the tests passing. Could you rebase it on the current master branch?

Also, if we evaluate c.to_json(validate="deep") on the above chart, an error is still raised, even though I think the chart itself is fine.

Note that this no longer happens after #2842 since we removed the deep parameter.

[ChristopherDavisUCI]

Thanks @joelostblom!

I think c.to_json(validate="deep") still exists even after #2842 (maybe it just doesn't get called automatically).

I will investigate a little more the origin of this wrapping. I think it first showed up in #646, but that seems like a pretty major PR, so it's a little hard for me to grasp at first glance.

Do you know, when I see an error in the automated tests like the following, is there a good way to tell which of the example files caused this error to be raised? Neither on GitHub nor locally with VS Code is it clear to me how to tell which example file was being used. (I tried running the test with debug mode in VS Code, but so far I have not gotten it to halt when the error is raised.)

It seems like code somewhere is expecting copy.condition to be a dictionary, where it is actually a _PropertySetter.

[mattijn]

Maybe add temporarily print(filename) around here while running the test in debug modus in VSCode?:

def test_render_examples_to_chart(syntax_module):
    for filename in iter_examples_filenames(syntax_module):
        print(filename)

The test_render_examples_to_chart function is defined using a @pytest.mark.parametrize(...), you can click right-mouse click on the green debug triangles and select which test within the parameterize you like to run in a debug session:

[ChristopherDavisUCI]

Thanks @mattijn! I haven't had a chance to experiment yet, but I should have time this week.

[ChristopherDavisUCI]

@joelostblom @mattijn I believe this is ready for review.

I found the source of the tests failing before... it was a copy.condition which needed to be changed to copy["condition"] because of the new property setters.

The main sales pitch for this PR is what was described in #2841 (comment). Briefly, I think if c.to_json() is a valid spec, then c.to_json(validate="deep") should not raise an error.

The main change in this PR is when an if_true condition like "Horsepower:Q" gets expanded from that shorthand notation. This PR makes this expansion occur earlier. I believe the functionality that's being replaced was introduced in #646. That PR is about shorthand parsing in general, not specifically related to conditions.

[joelostblom]

Why do we need this? If this PR removed the only place where Undefined was used, shouldn't we remove the import instead?

[ChristopherDavisUCI]

Thanks for checking on this @joelostblom. I left the Undefined import here because of this line: https://github.com/altair-viz/altair/blob/7bc754e8dd6748a8b4633b6f3cd2345ab7078fb2/altair/utils/__init__.py#L10

Do you think it's worth importing Undefined directly in that __init__ file instead? I haven't experimented at all, so it's possible other errors will show up if I move it.

[joelostblom]

Yes that would make more sense to me. Could you try that? If it doesn't work and seems laborious to resolve we can just keep it here, but let's give it a quick try first.

[ChristopherDavisUCI]

That seemed to work @joelostblom in c91468f
My quick tests did not catch any unwanted side effects.

[joelostblom]

LGTM, thank you!