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

Added test for pivot database setting #114

Merged
merged 1 commit into from Oct 17, 2016
Merged

Added test for pivot database setting #114

merged 1 commit into from Oct 17, 2016

Conversation

sroebert
Copy link
Member

This test shows that setting the database on the Pivot entity and swapping the generic entities, does not result in the same database.

@natebird
Copy link
Member

@sroebert, Can you provide a proposed change to the code to make this test pass?

@natebird natebird added the bug Something isn't working label Oct 12, 2016
@loganwright
Copy link
Member

@sroebert this is an interesting case, and I'm trying to consider potential solutions. I think as a human, I can tell that Pivot<Atom, Nucleus> and Pivot<Nucleus, Atom> should essentially be the same, but the compiler sees these as distinct types that must be accounted for separately. There might be some trickery we could do, just want to be careful that we don't make other things difficult.

I imagine the issue is that you're mistyping order sometimes and getting strange bugs, maybe we could suggest typealiasing in the docs, something like:

typealias NucleusAtomPivot = Pivot<Nucleus, Atom>

might be easier to remember across a project

@tanner0101
Copy link
Member

Thanks for submitting the failing test case! This shouldn't be too hard to fix. Just need to reference left vs. right for the database storage name.

@sroebert
Copy link
Member Author

Yeah that's what I thought as well, for now I was already putting a typealias to not have to retype the full pivot everywhere.

@tanner0101 tanner0101 merged commit 5c30919 into vapor:master Oct 17, 2016
@tanner0101
Copy link
Member

Merging this so I can resolve it on another branch. Thanks @sroebert!

@sroebert sroebert deleted the bug/pivot branch October 23, 2016 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants