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

Add += and -= support for Length (Issue #104) #107

Merged
merged 1 commit into from Oct 11, 2015
Merged

Conversation

@absoludity
Copy link
Contributor

absoludity commented Oct 10, 2015

More of a learning exercise for a first try of Rust, with a few questions :)

  • I noticed the current tests are all in one single test function. I've added a test for each operator I added here - is there any disadvantage to more unit-like tests?
  • When I run cargo test, all 23 tests pass, but the process doesn't exit successfully?
Process didn't exit successfully: `/home/michael/code/rust/euclid/target/debug/euclid-9d296e662001f41b` (signal: 4)

Caused by:
  Process didn't exit successfully: `/home/michael/code/rust/euclid/target/debug/euclid-9d296e662001f41b` (signal: 4)
  • I assume I should also (as another PR) add *= and /=, but currently there's not length * length (only length * scalefactor). Ah - is that because it'd be returning an area unit - so really just need length *= scalefactor and length /= scalefacter right? Anyway, I can add those, but was hoping to first understand more about what complex impl's like:
impl<Src, Dst, T: Clone + Div<T, Output=T>> Div<Length<Src, T>> for Length<Dst, T> {

are trying to declare...

Thanks

Review on Reviewable

@Manishearth Manishearth changed the title Add += and -= support for Length (Issue 104) Add += and -= support for Length (Issue #104) Oct 10, 2015
@jdm
Copy link
Member

jdm commented Oct 10, 2015

The problems with cargo output are worrying but there's nothing in this PR that should cause that. I'm going to try to kick the travis build and see what happens.

@jdm jdm closed this Oct 10, 2015
@jdm jdm reopened this Oct 10, 2015
@jdm
Copy link
Member

jdm commented Oct 10, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2015

📌 Commit 924f81f has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Oct 10, 2015

Testing commit 924f81f with merge 4e37e95...

bors-servo pushed a commit that referenced this pull request Oct 10, 2015
bors-servo
Add += and -= support for Length (Issue #104)

More of a learning exercise for a first try of Rust, with a few questions :)

* I noticed the current tests are all in one single test function. I've added a test for each operator I added here - is there any disadvantage to more unit-like tests?

* When I run `cargo test`, all 23 tests pass, but the process doesn't exit successfully?
```
Process didn't exit successfully: `/home/michael/code/rust/euclid/target/debug/euclid-9d296e662001f41b` (signal: 4)

Caused by:
  Process didn't exit successfully: `/home/michael/code/rust/euclid/target/debug/euclid-9d296e662001f41b` (signal: 4)
```
* I assume I should also (as another PR) add *= and /=, but currently there's not length * length (only length * scalefactor). Ah - is that because it'd be returning an area unit - so really just need length *= scalefactor and length /= scalefacter right? Anyway, I can add those, but was hoping to first understand more about what complex impl's like:

```
impl<Src, Dst, T: Clone + Div<T, Output=T>> Div<Length<Src, T>> for Length<Dst, T> {
```
are trying to declare...

Thanks

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/euclid/107)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 11, 2015

☀️ Test successful - travis

@bors-servo bors-servo merged commit 924f81f into servo:master Oct 11, 2015
1 of 2 checks passed
1 of 2 checks passed
homu Testing commit 924f81f with merge 4e37e95...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@absoludity
Copy link
Contributor Author

absoludity commented Oct 12, 2015

@jdm I'm not sure why you reopened this after merging - what else do you think should be added? I looked at *= and /= for Length, but afaict, they're not possible, as the (compile-time) type would need to change.

That is, if you look at the impl of Length * ScaleFactor , the output type for Length (Dst) is different to the input type (Src), or length_mm = length_m * 1000mm/m is fine, but length_m *= 1000mm/m isn't. Or if I'm missing something, just let me know. Thanks.

@jdm
Copy link
Member

jdm commented Oct 13, 2015

You got it backwards - I reopened it, and then it was merged successfully. This work is fine as it is :)

@absoludity
Copy link
Contributor Author

absoludity commented Oct 13, 2015

Ah - right. It's just the issue that is still open - #104 . Cheers.

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.