Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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 <johnson.alex.c@gmail.com>
@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
# for free to join this conversation on GitHub. Already have an account? # 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