-
Notifications
You must be signed in to change notification settings - Fork 51
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
fix rendering of joins #97
Conversation
Thanks for looking into this, @Philonous! I'm also wary of changing this behaviour, especially since it's possible to change the meaning of a query while shuffling parenthesis (source). I have a few questions I'd like to be answered before we land this:
|
Note to self: regardless of the answers to the questions above, it's probably going to be a good idea to ask people on the mailing lists to test their code against new |
@Philonous, after staring at your commits for some time, I think you got it right. But I don't feel too confident on my own judgement. Actually, I'm the one who introduced this bug in the first place ;-). So, I'd still appreciate if you could take a stab at the questions I've stated above. |
Edit: See 451beb9 for a test. The test fails without this patch and succeeds with it. 3 and 4) I think, but am not entirely certain, that the semantics of existing queries should be preserved as long as the constructors are changed to be left-associative. However, the type of expressions and patterns like (p |
+1 |
@Philonous, instead of editing older comments, in the future please write a new comment. I just saw that you wrote the test case due to @jonkri's comment. I don't get e-mail notifications for new commits nor comment edits. I'll merge and ask for feedback later, but thanks in advance! |
Oops sorry |
Great, thanks! Any idea when this could be merged and released onto Hackage? |
@Philonous, thank you for your PR, it's merged! I've bumped the version to @jonkri, I'm going to send a few e-mails asking people to test this new version. If everything goes well, I'll release it to Hackage in the next couple of weeks. Please let me know if it works for your codebase and if you had to make any adjustements for it to work. |
Tying the knot: https://groups.google.com/forum/#!topic/yesodweb/duwc5jcxgDY . |
Thanks! The sooner this can be released, the better it would be for us; we have some features lined up that would need this patch. :) For us, only a few type signatures changed, and so it was just a matter of deleting them and having GHC infer them. Take care! |
All fine for my biggest Esqueleto using project. |
Well, then I should probably mention that me and @Philonous are working on the same code base, so... :) |
I've released |
Great! Thanks! :) |
Esqueleto exhibits strange behaviour with respect to joins. All joins seem to be left-associated even though the join Constructors are right associative. Explicitly adding parenthesis to left-associate the constructors adds seemingly superfluous parenthesis to the generated SQL. On the other hand there is no way to generate right-associated joins in the SQL.
This pullrequest takes a stab at fixing this.
While the tests run though fine, I'm a bit unsure about this. I don't understand SQL well enough to be certain that this is correct in all cases.
This is related to #8
The downside of this patch is that the default associativity of the join operators now leads to different SQL code, e.g.
used to compile into
But after this patch it becomes
However, this is arguably the correct behaviour, as long as InnerJoin etc. are right-associative. Changing the associativity to infixl 2 restores the old behaviour and seems more intuitive to begin with, but can change the type of existing code.