Skip to content

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented Feb 2, 2018

This was a pretty straightforward rewrite. Overall I feel much better
about this code structure than what we had before. We're having to do a
bit of funkyness to avoid
rust-lang/rust#47941, but other than that
everything here was pretty straightforward. I had the UI tests guide me
the entire time.

@sgrif sgrif requested a review from a team February 2, 2018 21:19
@sgrif sgrif force-pushed the sg-rewrite-associations branch from bc664d7 to bf6d86d Compare February 3, 2018 13:19
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Looks fine module missing rustfmt
The missing tests as per #1484 should be done separated?

@sgrif
Copy link
Member Author

sgrif commented Feb 3, 2018

Yeah, I'll get to those soon. :)

This was a pretty straightforward rewrite. Overall I feel much better
about this code structure than what we had before. We're having to do a
bit of funkyness to avoid
rust-lang/rust#47941, but other than that
everything here was pretty straightforward. I had the UI tests guide me
the entire time.
@sgrif sgrif force-pushed the sg-rewrite-associations branch from bf6d86d to 5669551 Compare February 3, 2018 17:46
@sgrif sgrif merged commit 5669551 into master Feb 3, 2018
@sgrif sgrif deleted the sg-rewrite-associations branch February 3, 2018 17:46
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