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

Remove the “copy constructor” #132

Closed
wants to merge 6 commits into from
Closed

Remove the “copy constructor” #132

wants to merge 6 commits into from

Conversation

nbraud
Copy link
Collaborator

@nbraud nbraud commented Mar 23, 2019

Opened for discussion, folowing #23

  • Switch tests to using Vector2.convert where necessary
  • Update/improve the constructor tests
  • Finally, remove the ability to construct with Vector2(vector_like)

@nbraud
Copy link
Collaborator Author

nbraud commented Mar 27, 2019

Summarizing a discussion with @astronouth7303 :

  • I'd like not to have both Vector2.convert and Vector2(vector_like), especially when the idiomatic thing (the constructor) is the one with suboptimal behaviour.
  • We could implement __new__ to make constructing from a vector-like efficient.
  • Jamie pointed out that convert wasn't part of a stable release, so we can ditch it or make it private.

So I guess the 2 options I see, going forward, are:

  • ditch Vector2(vector_like) (this PR);
  • ditch convert, implement __new__.

How much of a performance and maintainance overhead is it to implement __new__ ourselves? (this is arcane magic I don't know yet)
Does implementing __new__ put expectations on subclasses of Vector2?

@pathunstrom
Copy link
Collaborator

It's possible to to write __new__ in such a way that as long as end users don't also modify it, will be stable. I've literally only needed to know __new__ once in my career, and strictly for interviews. It's not a feature I've used much.

@AstraLuma
Copy link
Member

__new__() should have approximately the same performance impact as __init__()

The flow is roughtly:

  1. type.__call__()
    1. self = type.__new__()
    2. self.__init__()

@nbraud
Copy link
Collaborator Author

nbraud commented Mar 29, 2019

OK, that sounds reasonable. Closing this in favour of implementing __new__.

@nbraud nbraud closed this Mar 29, 2019
@nbraud nbraud deleted the not-copy-ctor branch March 29, 2019 17:49
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.

3 participants