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

Make Parameterized.initialized private #745

Closed
wants to merge 1 commit into from

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Apr 17, 2023

I think this one slipped through the cracks until now, but it was noted in the Param 2.0 plan that it should be made private #543

@maximlt maximlt requested a review from jlstevens April 17, 2023 20:59
@maximlt
Copy link
Member Author

maximlt commented Apr 17, 2023

Hmm that's why I don't like so much internal attributes on a Parameterized class, they can easily clash with user code:

I really wonder what's the best way to avoid this kind of situation. Mangling? Relying on a private namespace a la .param?

@maximlt maximlt marked this pull request as draft April 17, 2023 21:04
@jlstevens
Copy link
Contributor

Thanks for this!

As you know, this particular issue recently bit me.

I think both initialized and _initialized are common and likely to clash. As you say, attributes on parameterized classes often cause clashing namespaces: for this reason I suggest replace .initialized with something even less likely to be problematic e.g. _initialized_ (two underscores should do the trick!)

@hoxbro
Copy link
Member

hoxbro commented May 2, 2023

I agree with @jlstevens about the high likelihood of a name clash, though I would suggest __initialized instead. This is because I never really look at the end of the variable but always at the start.

@maximlt
Copy link
Member Author

maximlt commented May 2, 2023

I agree with @jlstevens about the high likelihood of a name clash, though I would suggest __initialized instead. This is because I never really look at the end of the variable but always at the start.

The way attributes are sometimes read from an instance or from a class makes relying on mangling a little tricky, it could work though, I just need to try. Generally, I'm more in favor of looking for a way to hide all the private attributes under a private namespace, a la .param, so that we can document and protect only a few attributes.

@jlstevens
Copy link
Contributor

jlstevens commented May 2, 2023

__initialized is also fine and if there were a way to namespace the private attributes (on something double underscored) that would be even better! (Oh and yes, I do know that name mangling does make it trickier to implement)

@jbednar
Copy link
Member

jbednar commented May 12, 2023

I agree with the goal of this PR, but I also agree with the observation that _initialization is also a common idiom, and so I'm not sure it's the right fix. __initialized seems fine if it can be made to work, or else putting such attributes under .param in in a very obscurely named attribute seems better.

@jlstevens
Copy link
Contributor

If sticking _initialized on the param namespace object works and is easier than dealing with name mangling, I think that would be a big improvement.

@maximlt
Copy link
Member Author

maximlt commented Jul 11, 2023

Superseded by #766

@maximlt maximlt closed this Jul 11, 2023
@maximlt maximlt deleted the make_initialized_private branch July 11, 2023 17:10
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.

4 participants