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

Add __or__, __ror__, __ior__ methods to BasePlotlyType to support dict-like updates via pipe operator #4047

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

janosh
Copy link
Contributor

@janosh janosh commented Jan 30, 2023

@nicolaskruchten I'm not very familiar with the plotly code base so feel a bit like an elephant in a porcelain shop. But maybe this is first step in the right direction for #4046. Just a rough first draft to solicit feedback at this point. No tests yet.

Closes #4046

@alexcjohnson
Copy link
Collaborator

@janosh this looks great, apologies for taking so long to review. One question: does this apply magic underscores? ie if we do things like fig.layout.update(margin_pad=4) do we get {'margin': {'pad': 4}}?

No comments on the code, looks good to me. Just need some tests I guess in test_update_objects, and a changelog entry (after merging master in, which should get the tests passing again).

@gvwilson gvwilson requested a review from marthacryan July 19, 2024 15:30
@gvwilson gvwilson added feature something new P2 considered for next cycle community community contribution labels Aug 12, 2024
Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

Nice PR and clever syntax improvement. It'd be nice to accompany this with documentation if merged. Definitely could do for some tests to verify that this works 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support dict union operator in legend updates
5 participants