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

Privatize Widgets.widget #3122

Merged
merged 2 commits into from Feb 16, 2021
Merged

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Feb 16, 2021

Closes: #2944

Using such a generic name could lead to collisions with user code as in the linked issue. Here it is changed to a name far less likely to collide with a user subclass and privatized. I'm less sure about making it private but it seems as though doing this will increase future flexibility and clean up what shows up when tab completing on a widget subclass.

I also gave the same treatment to widget_types for consistency.

@ianhi ianhi added this to the 8.0 milestone Feb 16, 2021
@jasongrout
Copy link
Member

Thanks!

@jasongrout jasongrout merged commit 5de5abe into jupyter-widgets:master Feb 16, 2021
@ianhi ianhi deleted the global-list branch February 17, 2021 00:20
@vidartf
Copy link
Member

vidartf commented Feb 19, 2021

So is this still safe to be considered "protected" then? I.e. subclasses can rely on this property being available with the current variable name until the next major release ?

@vidartf
Copy link
Member

vidartf commented Feb 19, 2021

I mean, the fact that the embed code now access a "private" variable seems hacky.

@ianhi
Copy link
Contributor Author

ianhi commented Mar 12, 2021

So is this still safe to be considered "protected" then? I.e. subclasses can rely on this property being available with the current variable name until the next major release ?

I mean, the fact that the embed code now access a "private" variable seems hacky.

My understanding of private variables was that it's ok for anything in the library they are in to use them, including subclasses. In which case it is not hacky for the embed code to access it. Is that different than the canonical view? I'm totally happy to revisit this if you have a suggestion for what a better approach is.

@pllim
Copy link
Contributor

pllim commented Aug 22, 2022

Looks like this was "privatized" without any deprecation period. Broke our code downstream. FYI.

@jasongrout
Copy link
Member

Looks like this was "privatized" without any deprecation period. Broke our code downstream. FYI.

Thanks for pointing this out. At the very least, we should note this in the migration documentation.

@maartenbreddels
Copy link
Member

Yeah, I think .widgets is used widely, and this will at least break a lot of my code. I understand the change, but maybe we could at least provide a class based getter such as in:
widgetti/reacton@6fb0784

If a subclass decides to use `.widgets', yes it will break code that assumed it to the dict of widgets, but it will make most existing code out there still work.

maartenbreddels added a commit to maartenbreddels/ipywidgets that referenced this pull request Aug 24, 2022
In jupyter-widgets#3122 we renamed .widget and .widget_types to ._active_widgets
and ._widget_types. That breaks code, and we did not have a deprecation
period.
This PR makes the dict and registry non-members of the Widget class
and puts in a backwards compatible way the deprecated these members.
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.

Widgets are not unique when stored in a dict in sub-class
5 participants