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

Replace '__new__' with '__init__' in CubeList creation. #3264

Closed
wants to merge 1 commit into from

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Feb 7, 2019

"… and typecheck content."

Raised in the course of #3238

This is intended to combine with that, so

  • (a) no whatsnew, though it is also applying that newly strict "only Cubes in CubeLists" approach
  • (b) doesn't anticipate other stuff there, like possibly a better 'is a Cube' test

@bjlittle
Copy link
Member

@pp-mo Would you mind giving me an executive summary for the context of this PR? What's the purpose and intent?

Much obliged, thanks 😄

@pp-mo
Copy link
Member Author

pp-mo commented Oct 23, 2019

@bjlittle executive summary ... purpose and intent?

The existing __new__ method is clearly intended to ensure that CubeLists can only contains Cubes when initially created.
But it is broken, and doesn't do that.

Because ... For reasons I don't quite grasp, the line cube_list = list.__new__(cls, list_of_cubes) always returns a new empty list object. So the subsequent checking code does nothing, ever.

I'm not even clear why this ever had __new__ rather than an __init__ method, as none of the usual reasons to use a __new__ method apply : It is required when you need to have object creation return an existing object, or a different type etc. : anything other than simply producing a new object of the expected class.
This code goes right back to the earliest GitHub commit. The choice was possibly related to the really old inability to subclass builtin container types prior to python 2.2.

The proposed replacement removes the unnecessary use of __new__, and does the checking job right, disallowing CubeList creation with non-cube arguments.
However, substantially more work is required to prevent all other possible ways of getting a non-cube in a CubeList.

@rcomer
Copy link
Member

rcomer commented Oct 21, 2020

This is intended to combine with [#3238]

I gave up on #3238 as it had stalled for some time. It was something of a sledgehammer to crack a nut so not obviously the right way to go anyway.

Probably still worth ripping out CubeList.__new__ in the interests of tidying up though?

@pp-mo
Copy link
Member Author

pp-mo commented Oct 28, 2020

Rebased.
Hoping it goes ...

@rcomer rcomer mentioned this pull request Mar 29, 2021
@rcomer rcomer linked an issue Mar 29, 2021 that may be closed by this pull request
@pp-mo pp-mo closed this Mar 18, 2022
@pp-mo pp-mo deleted the cubelist_new branch March 18, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CubeList instantiation
6 participants