Remove Changeset
#1486
Merged
Remove Changeset
#1486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a long overdue refactoring. I'm going to do a similar refactoring for `InsertValues` as part of #1106, but this shares a lot of groundwork for that and is much smaller in scope. `Changeset` has always been entirely redundant with `QueryFragment`, but we had to keep it as a separate trait for two places where the implementation changed: - `Eq` doesn't qualify the column name - tuples skip commas when `is_noop` returned true (for `Changeset` this was only the case for `None` or a tuple where all elements were `None`) The first one is easy to solve, we just return a type other than `Eq` in `AsChangeset` (there was very little reason to use `Eq` in the first place. We weren't able to re-use any of its code.) The second one is a bit trickier, since we need to replicate the `is_noop` method in `QueryFragment`. I'm fine with doing this, since `InsertValues` has the same method (and tuples implement `walk_ast` identically for `InsertValues` and `Changeset`), so it makes sense to share them. Originally I added an explicit `noop` method to `AstPass` which we would call. However, it felt weird that the impl for `()` which literally just does `Ok(())` didn't call `noop`. At that point I realized I could just generalize this to "did a method that generates SQL get called?" This works fine for updates. I'm not 100% sure if it'll work for inserts, but it's worth a shot. I should note that the semantics of the new `is_noop` are slightly different than the one on `InsertValues`, which explicitly states that an empty array will return `false`. This will make it return `true` when we switch inserts to use this. Since we already have to explicitly handle empty arrays long before we start checking `is_noop`, I think that's fine.
weiznich
approved these changes
Jan 17, 2018
| None => Ok(()), | ||
| fn as_changeset(self) -> Self::Changeset { | ||
| Assign { | ||
| _column: self.left, |
There was a problem hiding this comment.
Why does this use a underscore name?
There was a problem hiding this comment.
It's never actually used. Only the type is. I didn't feel like using PhantomData for it since we actually have the value at the point of construction.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a long overdue refactoring. I'm going to do a similar
refactoring for
InsertValuesas part of #1106, but this shares a lotof groundwork for that and is much smaller in scope.
Changesethasalways been entirely redundant with
QueryFragment, but we had to keepit as a separate trait for two places where the implementation changed:
Eqdoesn't qualify the column nameis_noopreturned true (forChangesetthiswas only the case for
Noneor a tuple where all elements wereNone)The first one is easy to solve, we just return a type other than
EqinAsChangeset(there was very little reason to useEqin the firstplace. We weren't able to re-use any of its code.)
The second one is a bit trickier, since we need to replicate the
is_noopmethod inQueryFragment. I'm fine with doing this, sinceInsertValueshas the same method (and tuples implementwalk_astidentically for
InsertValuesandChangeset), so it makes sense toshare them.
Originally I added an explicit
noopmethod toAstPasswhich we wouldcall. However, it felt weird that the impl for
()which literally justdoes
Ok(())didn't callnoop. At that point I realized I could justgeneralize this to "did a method that generates SQL get called?" This
works fine for updates. I'm not 100% sure if it'll work for inserts, but
it's worth a shot.
I should note that the semantics of the new
is_noopare slightlydifferent than the one on
InsertValues, which explicitly states thatan empty array will return
false. This will make it returntruewhenwe switch inserts to use this. Since we already have to explicitly
handle empty arrays long before we start checking
is_noop, I thinkthat's fine.