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

Allow Vector-likes in all operations #56

Merged
merged 11 commits into from
Oct 13, 2018
Merged

Allow Vector-likes in all operations #56

merged 11 commits into from
Oct 13, 2018

Conversation

AstraLuma
Copy link
Member

Make all operations work with vector-likes.

@pathunstrom
Copy link
Collaborator

I like where we're going with this.

Suggesting we put most of this under a class method .from(value: Union[Sequence, Mapping[str, numbers]]) that just checks length then tries to instantiate and raises a Value error if it fails. Most of the logic is clear: Must be length 2, and look like a vector (in the case of maps), and then we can get a meaningful error everywhere, also provides a utility for people doing things programatically.

@AstraLuma
Copy link
Member Author

Standard python is that conversion should be in the constructor, ie Vector2({Vector-like}), but otherwise yes.

@pathunstrom
Copy link
Collaborator

I only suggest a class method because the constructor is Vector2(x, y) and I want to keep that as a general rule. Class method as an alternative constructor is pretty well established pattern (see int.from_bytes).

@@ -67,16 +84,18 @@ def __getitem__(self, item):
raise TypeError

def __repr__(self):
return "Vector2({}, {})".format(self.x, self.y)
return "{}({}, {})".format(type(self).__name__, self.x, self.y)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we're modifying this, lets switch to F-strings.

@@ -115,6 +134,7 @@ def reflect(self, surface_normal: 'Vector2') -> 'Vector2':
"""
Calculate the reflection of the vector against a given surface normal
"""
surface_normal = _mkvector(surface_normal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do this, we need to change the type hint.

On that front, we should probably build a union or type var for this.

@AstraLuma
Copy link
Member Author

Addressed everything.

Not super happy with the name .convert(), but I couldn't come up with a better one.

@pathunstrom
Copy link
Collaborator

I would have just used Vector2.from(vector_like) but convert is fine for now.

@AstraLuma
Copy link
Member Author

from is a keyword; can't be used as method names.

@AstraLuma AstraLuma merged commit b5e574b into master Oct 13, 2018
@AstraLuma AstraLuma deleted the vector-like branch October 13, 2018 03:31
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.

2 participants