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

Update mint branch to be compatible with euclid master #302

Closed
wants to merge 8 commits into from

Conversation

@icefoxen
Copy link

icefoxen commented Aug 2, 2018

Also updates mint to 0.5.1. The only real difference is changing Frommint::RowMatrix2x3 for TypedTransform2D to Frommint::RowMatrix3x2. This may or may not be 100% valid since my understanding of the math and how it all fits together isn't GREAT, but...

Should supercede #205


This change is Reviewable

kvark and others added 5 commits Jun 2, 2017
The only real difference is changing `From<mint::RowMatrix2x3> for
TypedTransform2D` to `From<mint::RowMatrix3x2>`.  This may or may
not be 100% valid since my understanding of the math and how it all
fits together isn't GREAT, but...
@kvark
kvark approved these changes Aug 3, 2018
Copy link
Member

kvark left a comment

LGTM, thanks for reviving this!
It would be good to rebase this instead of adding a merge commit.

Cargo.toml Outdated
@@ -15,6 +15,7 @@ unstable = []
[dependencies]
num-traits = { version = "0.2" }
serde = { version = "1.0", features = ["serde_derive"], optional = true }
mint = {version = "^0.5.1", optional = true}

This comment has been minimized.

@kvark

kvark Aug 3, 2018

Member

the ^ here shouldn't be necessary

@nical
Copy link
Collaborator

nical commented Aug 3, 2018

LGTM

@kvark
Copy link
Member

kvark commented Aug 8, 2018

@icefoxen ping about rebasing and nits

@icefoxen
Copy link
Author

icefoxen commented Aug 9, 2018

@kvark shoot, slipped my mind. I'll get it done Hopefully Soon.

icefoxen added 2 commits Aug 16, 2018
@icefoxen icefoxen mentioned this pull request Aug 16, 2018
@icefoxen
Copy link
Author

icefoxen commented Aug 16, 2018

Rebased in #303 . I can try to push it to this branch if you want?

@kvark
Copy link
Member

kvark commented Aug 16, 2018

@icefoxen yeah, I don't think it makes sense to file a new PR just for the rebase.

@icefoxen
Copy link
Author

icefoxen commented Aug 17, 2018

done

@kvark
Copy link
Member

kvark commented Aug 17, 2018

@icefoxen there shouldn't be any rebase merge commits. Ideally, just a single fix commit after my original ones. Could you rebase some more please?

@icefoxen
Copy link
Author

icefoxen commented Aug 17, 2018

No. I'm sick of this song and dance and did exactly what you wanted me to: pushed the rebased commit chain to the original branch so we can use this PR. If you want a clean version, re-open and merge #303.

I'm sorry, but we've spent FAR longer fucking around with git than it actually took to write the PR. I KNOW the answer is, literally, "git gud", and I'm trying 'cause I know in some sorts of development it's actually important, but at some point even my patience for procedure over actual content comes to an end. All the code is there. You want it to look a particular way, fix it yourself.

@kvark
Copy link
Member

kvark commented Aug 17, 2018

@icefoxen I'm sorry about your frustration with git and my update requests. It doesn't matter that you spent more time learning git than doing the PR because that knowledge is going to stay with you in the future. I'll get your commits squashed and appended to my PR as requested (by myself).

@kvark
Copy link
Member

kvark commented Aug 17, 2018

Closing in favor of #205

Note to @icefoxen :
With merge commits, doing my regular git pull -r upstream master would require resolving everything from scratch, which isn't the greatest option. What I ended up doing was:

git clone https://github.com/servo/euclid
cd euclid
git fetch https://github.com/icefoxen mint
git checkout FETCH_HEAD
git checkout -b mint
git reset --soft master
git commit -m "Mint integration"
git push -f origin HEAD
@kvark kvark closed this Aug 17, 2018
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.