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

Use verbs instead of adjectives for transformations with #[must_use]. #198

Merged
merged 1 commit into from May 29, 2017

Conversation

@nical
Copy link
Collaborator

nical commented May 27, 2017

Fixes #195.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented May 27, 2017

r? @kvark

@kvark kvark self-requested a review May 29, 2017
Copy link
Member

kvark left a comment

Wouldn't it be easier to just put #[must_use] on types instead of trying to cover all the methods?

@@ -179,12 +183,12 @@ where T: Copy + Clone +
}

/// Applies a scale after self's transformation and returns the resulting transform.
pub fn post_scaled(&self, x: T, y: T) -> TypedTransform2D<T, Src, Dst> {
pub fn post_scale(&self, x: T, y: T) -> TypedTransform2D<T, Src, Dst> {

This comment has been minimized.

@kvark

kvark May 29, 2017

Member

missing must_use here as well?

@nical nical force-pushed the nical:adjective-form branch from 1e3542c to 8dceb6d May 29, 2017
@nical nical force-pushed the nical:adjective-form branch from 8dceb6d to 137660d May 29, 2017
@nical
Copy link
Collaborator Author

nical commented May 29, 2017

must_use comes from the way the functions are written in euclid (only immutable methods that return a result), rather than the structures themselves. Unless you have a strong preference I would rather keep the annotations on the methods even if it is a bit noisy. That way users of euclid can return vectors and points in places where must_use does not make as much sense as it does here.

@kvark
Copy link
Member

kvark commented May 29, 2017

Sounds good, thanks @nical !
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented May 29, 2017

📌 Commit 137660d has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented May 29, 2017

Testing commit 137660d with merge 13b3ddc...

bors-servo added a commit that referenced this pull request May 29, 2017
Use verbs instead of adjectives for transformations with #[must_use].

Fixes #195.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/198)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 29, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing 13b3ddc to master...

@bors-servo bors-servo merged commit 137660d into servo:master May 29, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.