-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Deep magic underscore error messages #2824
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
Conversation
when deep underscore error messages are implemented
This gets the path to the property as a tuple like _str_to_dict_path used to do, but also returns the indices of each property in the string.
@@ -2,6 +2,8 @@ | |||
import json as _json | |||
import sys | |||
import re | |||
from functools import reduce | |||
from numpy import cumsum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure we want to import numpy
here ... it's not a hard dependency of plotly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, numpy shouldn't be a hard dependency here. Here's an alternative approach you can use for cumsum if you need it (https://realpython.com/python-reduce-function/#comparing-reduce-and-accumulate).
try: | ||
fig2 = go.Figure(layout_title_txt="two") | ||
except ValueError as e: | ||
assert ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for these assertions, it would be a lot more readable and future-proof to assert that the error message from go.Figure(layout_title_txt="two")
is simply the same as the error message from go.Figure(layout_title = dict(txt="two"))
, rather than doing assertions on the string itself, that way if and when we change the string we have fewer places to check :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another case for with pytest.raises
: https://docs.pytest.org/en/stable/assert.html#assertions-about-expected-exceptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jon! I should close this PR, it's been superseded by this one: #2843
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
When a user tries to write to a bad path, a string describing where in the path the error occurred is appended to the error message. Not all the tests are passing because we also want to raise an appropriate error related to the last valid object in the path. For example if walking down the path we get to a Line object, but then fail when trying to look up something in it, we want to list its valid keys.
The exception types are not the ones we original expected. TODO: Probably the exceptions should be compared rather than the strings.
Otherwise an infinite loop will ensue, because the original path will get mapped over and over again. Converting the original path in to the parts of the mapped path superated by underscores breaks this recursion.
the latter one prints nicer error messages for the user.
It is clearer now how the keys are tested and optionally updated for BaseLayoutTypes. The code is more compact too.
to avoid importing numpy
The one failure of |
This way they are guaranteed to match errors thrown when no deep paths are used, which is what we want.
Other than the one failure, which should be resolved once plotly.js 1.57 is out, and the hline vline PR is merged, this PR is ready for review 👍 |
Please merge master into this PR or rebase this one onto master :) |
There are still 800 modified files here, so probably more merge/rebase/squash work is needed :) |
Addresses #2072
Not finished yet, just putting up so others can review and comment.