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

Raise error when iterating over patch objects. #2513

Merged
merged 6 commits into from
May 19, 2023
Merged

Raise error when iterating over patch objects. #2513

merged 6 commits into from
May 19, 2023

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Apr 24, 2023

Fix #2512

@T4rk1n T4rk1n requested a review from alexcjohnson as a code owner April 24, 2023 16:20
dash/_patch.py Outdated Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

@T4rk1n what's the issue with raising on __repr__? Not seeing a problem in a simple test but I haven't run the full test suite on it. Anyway raising on __iter__ is nice, we should probably also do __contains__ (currently 5 in Patch().x is an infinite loop) and maybe __bool__ (a patch is always truthy), __eq__, __ne__ (it will not equal the value it has in the actual prop) - ... but those are all very specific usage cases, it would be nice to do something better for folks just trying to print a piece of the prop.

Actually perhaps better than raising on __repr__ would be just a more descriptive string, something like:

<write-only dash.Patch object at ['layout', 'title', 'font', 'color']>

Another related thing I'm noticing, if someone tries to use part of a prop to set a different part, like this:

patched_figure = Patch()
# seems reasonable, make the figure square?
patched_figure["layout"]["height"] = patched_figure["layout"]["width"]
return patched_figure

it leads to infinite recursion inside clean_to_json_compatible in the plotly.py JSON encoder - any way we can break that?

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Apr 27, 2023

what's the issue with raising on repr?

Not sure I understand why raise on repr? That is for description of the object so maybe add it like you propose is good.

Co-authored-by: Alex Johnson <[email protected]>
@T4rk1n
Copy link
Contributor Author

T4rk1n commented Apr 27, 2023

it leads to infinite recursion inside clean_to_json_compatible in the plotly.py JSON encoder - any way we can break that?

Think that will require more thought, maybe check the value in setattr if it's another patch and prevent that?

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Apr 27, 2023

and maybe bool

Equality comparators may be used with a filtering system so let's leave them out for now.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Probably more we could do here, but you're right we should be cautious in case we add other functionality later.

# Conflicts:
#	CHANGELOG.md
@T4rk1n T4rk1n merged commit a8b3ddb into dev May 19, 2023
@T4rk1n T4rk1n deleted the fix/#2512 branch May 19, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Exception when property of patched_fig is viewed
2 participants