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

Add a function for SQL EXISTS expressions. #534

Merged
merged 1 commit into from
Dec 8, 2016
Merged

Add a function for SQL EXISTS expressions. #534

merged 1 commit into from
Dec 8, 2016

Conversation

sgrif
Copy link
Member

@sgrif sgrif commented Dec 8, 2016

While this is useful for some cases where you don't want to load the
rows, this won't fill every use case for the expression, as right now
you wouldn't be able to build a query that references the outer table.
For us to do that and have it be type safe we'd need overlapping impls
for SelectableExpression (story of my life), which requires
rust-lang/rust#29864 being implemented.

Fixes #414.

@sgrif sgrif requested a review from killercup December 8, 2016 17:29
Copy link
Member

@killercup killercup left a comment

Choose a reason for hiding this comment

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

LGTM with doc nits fixed

use result::QueryResult;
use types::Bool;

/// Creates a SQL `EXISTS` expression. The argument must be a complete SQL
Copy link
Member

Choose a reason for hiding this comment

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

Start new paragraph after first sentence for nice looking docs :)

/// usefulness.
///
/// # Example
/// ```rust
Copy link
Member

Choose a reason for hiding this comment

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

Add newline after headline


fn collect_binds(&self, out: &mut DB::BindCollector) -> QueryResult<()> {
try!(self.0.collect_binds(out));
Ok(())
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the inner self.0.collect_binds(out) suffice here without the try and Ok unit stuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I quite frequently end up adding fields to these things, so I've been consistently writing it like this (and never relying on the return value of the last delegating line) to make it easier to add more fields in the future with less diff noise

Copy link
Member

Choose a reason for hiding this comment

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

Good point. I appreciate les diff noise :)

While this is useful for some cases where you don't want to load the
rows, this won't fill every use case for the expression, as right now
you wouldn't be able to build a query that references the outer table.
For us to do that and have it be type safe we'd need overlapping impls
for `SelectableExpression` (story of my life), which requires
rust-lang/rust#29864 being implemented.

Fixes #414.
@sgrif sgrif merged commit 9819ed4 into master Dec 8, 2016
@sgrif sgrif deleted the sg-exists branch December 8, 2016 19:29
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