Deep magic underscore error messages rebase

[nicholas-esterer]

See #2072

Rebase of

#2824

[nicolaskruchten]

please revert these whitespace-only changes ... I think your editor must be doing this?

[nicholas-esterer]

This is weird because I never opened this file, so maybe black did it. I'll just check out the master version.

[nicolaskruchten]

Ah, so every test here needs to be rewritten to use with pytest.raises(...) because otherwise these tests can pass if no error is raised! We need to guarantee that the error is raised here, and check its contents.

[nicholas-esterer]

I tried using pytest.raisesRegex because I wanted to check the error message, but had trouble making a regex matching multiple lines. I can show you the error when using the multiline flag for re, it kind of looked like a bug with re. Also I don't need a full-blown regex, just a string comparison would suffice.

[nicolaskruchten]

So the error message for this call has changed for the worse IMO:

import plotly.graph_objects as go
go.Figure(go.Scatter()).update_traces(text_yo="hey")

It used to say basically text_yo isn't valid and now it drills into text and says "I can't do anything with yo". Maybe we need specific handling for leaves here?

[nicolaskruchten]

Also needs a changelog entry

[nicholas-esterer]

So the error message for this call has changed for the worse IMO:

import plotly.graph_objects as go
go.Figure(go.Scatter()).update_traces(text_yo="hey")

It used to say basically text_yo isn't valid and now it drills into text and says "I can't do anything with yo". Maybe we need specific handling for leaves here?

Do you mean this error message is not so great?:
TypeError: 'NoneType' object is not subscriptable
I agree but I guess I don't understand why it doesn't work like for the other cases. For example, this doesn't fail:

import plotly.graph_objects as go
go.Figure(go.Scatter()).update_traces(text="hey")

Could it be because text doesn't have a corresponding go.layout.Text object? I'll have to look.

[nicolaskruchten]

This is an improvement, but the error message is still a little opaque. fig.update_layout(width_x=1) gives:

Property does not support subscripting:
width_x
~~~~~

I wonder if it shouldn't be similar to the error we get if we try fig.update_layout(width="a") which gives:

    Invalid value of type 'builtins.str' received for the 'width' property of layout
        Received value: 'a'

    The 'width' property is a number and may be specified as:
      - An int or float in the interval [10, inf]

Perhaps the width_x case could return:

    Invalid value received for the 'width' property of layout

    The 'width' property is a number and may be specified as:
      - An int or float in the interval [10, inf]

Property does not support subscripting:
width_x
~~~~~

[nicolaskruchten]

@jonmmease mind taking a quick look here to see if everything makes sense?

[jonmmease]

Looks really nice! I left a couple of comments on the exception class hierarchy. I still want to check out the branch and play around with it a little. I'll do that today.

[jonmmease]

This is technically a breaking change unfortunately, since this will behave differently:

try:
    fig.update(...)
except KeyError:
    pass

Instead, I think you should be able to subclass KeyError and override the __str__ magic method.

[jonmmease]

Right now it looks like trailing underscores are ignored, e.g. this is valid:

fig.update_layout(width_=300)

I'm torn on whether this should raise an error (looks like it did before).

[jonmmease]

I agree with @nicolaskruchten that the width_x error message is a bit awkward. But I don't have a better suggestion at the moment:

This one is also a little off IMO:

fig.update_layout(title_text_asdf=300)

TypeError: string indices must be integers
Bad property path:
title_text_asdf

[nicholas-esterer]

Do you have the latest commit? For me I get this, which was desired:

TypeError: 'NoneType' object is not subscriptable

Invalid value received for the 'text' property of layout.title

    The 'text' property is a string and must be specified as:
      - A string
      - A number that will be converted to a string

Property does not support subscripting:
title_text_asdf
      ~~~~

[nicholas-esterer]

But I'm also open to that being problematic : )

[nicholas-esterer]

Thanks @jonmmease and @nicolaskruchten for the reviews!

How important is it to maintain KeyError over ValueError?
Before this PR, this is the error that was thrown when calling __getitem__ on a subclass of BasePlotlyType with a property of length 1 (e.g., go.Scatter()['doesntexist']). The reason I changed this to call self._raise_on_invalid_property_error(prop) is because this latter function gives a more verbose error message which lists the valid properties. That way, after walking down the path described by the magic underscores, the last error encountered would be this error and it would list the valid properties.

Now we could change self._raise_on_invalid_property_error(prop) to raise KeyError, because that's actually a more relevant Exception, but then I reckon we're changing the API in more places (and breaking more tests).
The way to keep everything the same is to add an argument that causes it to raise the desired exception, say self._raise_on_invalid_property_error(prop,error_type=KeyError). The difficulty of that is knowing when to pass that argument in, because currently this function can't know if it was called with a property with or without underscores because in the underscore case, the property is split into single properties.

You may ask: "But how did the more verbose error get thrown before this PR?"
It got thrown only on property assignment and not on property lookup. The original desire of this PR was to have

go.Scatter(x=[1,2],y=[3,4],line_colr='blue')

show the possible values for plotly.graph_objs.scatter.Line and not scatter. The way that's done is properties are looked up in order to descend the property tree (here 'line' is looked up in go.Scatter and colr is looked up in the resulting plotly.graph_objs.scatter.Line object: this last lookup fails). So I wanted to get that failure to give a better error message and so now lookup calls self._raise_on_invalid_property_error(prop) and doesn't just throw KeyError.
Before this PR,

go.Scatter(x=[1,2],y=[3,4],line_colr='blue')

basically did this, because it knew x and y were in go.Scatter (they are parameters to its __init__ function)

s = go.Scatter(x=[1,2],y=[3,4])
if 'line_colr' not in s:
    #... raise some error relative to `s`

and so that's why the error was reported relative to go.Scatter and not plotly.graph_objs.scatter.Line.
I didn't want to change how __contains__ works (notice it returns True or False and doesn't raise an exception) because in order to get where the in call failed an exception would have to be thrown and then that exception would contain the information. I wagered that possibly throwing exceptions every time in was called on plotly objects would be disastrous.
So that is what led to the current implementation, which throws the exceptions when __getattr__ fails. Notice with that change, I only had to change 1 previous test 😌 : the one that threw KeyError instead of ValueError.

[nicolaskruchten]

@nicholas-esterer I think I agree with Jon that changing the KeyError type is a breaking change. We do want the nicer error message but without changing the type. Can we not catch the ValueError internally right before throwing in in the [] case and rewrap it in a KeyError with the nicer message?

[nicholas-esterer]

Seeing what happens if we change BasePlotlyType._raise_on_invalid_property_error to raise PlotlyKeyError 🤞
But this also breaks the API.

[nicolaskruchten]

By the [] case, you mean when a user does:some_obj['badprop']? No, because the routine that walks down the property tree also has to use [].

Yes, that's the case I meant. OK, then how about we leave that one case to return a KeyError (albeit with a nicer error message), and catch that KeyError and rewrap it as a ValueError in the non-[] cases?

Either way, I think Jon and I are aligned that we should not change the error types in this PR. I agree that the current situation is not ideal, with KeyError sometimes and ValueError sometimes, but it's too late to change this basically :)

[nicholas-esterer]

I agree too that changing the error types is a bad idea. I guess I got to start digging 🕳️ 🙂

[nicholas-esterer]

OK! So I think I've managed to preserve the previous exception behaviour as I have not changed any of the already existing tests (the only test "changed" is packages/python/plotly/plotly/tests/test_core/test_errors/test_dict_path_errors.py and it's the tests I wrote for this PR).

[nicholas-esterer]

Now subscripting errors are thrown for any type throwing TypeError when subscripting them. This is enough for Plotly.py purposes because other than dicts and BasePlotlyType-dervived objects (which behave much like dicts), the objects found at keys are None, str or numbers, which throw TypeError when you try and use dict-like lookup (e.g., 1['badkey']). These errors are cast to ValueError for assignment and PlotlyKeyError for lookup.

[nicolaskruchten]

This looks like it's getting close to mergeable! Can we switch to using the full-width ^ highlighting for both cases here rather than single-^ and full-width ~ please?

[nicolaskruchten]

please move this to the 4.13 block

[nicolaskruchten]

The only outstanding item here seems to be the "trailing underscore" question... Right now this fails on master and succeeds in this branch: fig.update_layout(title_text____="test")

[nicolaskruchten]

Ahh actually it's a bit worse... this also works: fig.update_layout(__title__text__=300)

[nicolaskruchten]

Cool, just need some tests on the leading/trailing/multiple underscores :)

[nicolaskruchten]

@jonmmease this is ready for another review I think. Check out the "did you mean...?" feature!!

[jonmmease]

Looks great @nicholas-esterer. I'm happy with the code, so if @nicolaskruchten is happy with the behavior (I didn't check out and play with the latest version) then lgtm.