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

insert is inconsistent, and should be changed to match other operations #30

Closed
sgrif opened this issue Nov 29, 2015 · 3 comments
Closed

Comments

@sgrif
Copy link
Member

sgrif commented Nov 29, 2015

This is just an artifact of the fact that I implemented this before everything other than the most basic forms of select. This should be changed to match update and delete. This is the API I'm imagining:

insert_into(table).values(records)

This would return a command that can be used in the same way the return values of update and delete, which will be less painful with #29.

Since this represents a breaking change in one of the most common APIs, we should do this sooner rather than later.

@sgrif sgrif added this to the 0.2 milestone Nov 29, 2015
sgrif added a commit that referenced this issue Nov 29, 2015
Writing this got me thinking about some API changes for consistency.
See #29 and #30.
@mcasper
Copy link
Contributor

mcasper commented Nov 29, 2015

How would we feel about reversing the order?

insert(record).into(table)

It feels more consistent to me with update and delete, which both seem to follow the pattern of

action(record).action_specific(parameter)

So we would be left with the API

insert(record).into(table)
update(record).set(changes)
delete(record)

@sgrif
Copy link
Member Author

sgrif commented Nov 29, 2015

Would it though?

update(users.filter(id.eq(1))).set(user_changes)
delete(users.filter(id.eq(1))
insert_into(users).values(new_user)

vs

update(users.filter(id.eq(1))).set(user_changes)
delete(users.filter(id.eq(1))
insert(new_user).into(users)

there's also the third option (not mutually exclusive)

new_user.insert_into(users)

@sgrif
Copy link
Member Author

sgrif commented Nov 29, 2015

I do want to end up supporting update(record) and delete(record) as sugar for filtering on the PK though, so there's merit to insert.into. It also reads more naturally.

@sgrif sgrif removed the enhancement label Nov 29, 2015
sgrif referenced this issue Nov 30, 2015
Through usage, it has become clear that in the overwhelming majority of
cases, you either expect N records, or exactly 1. Expecting either 0 or
1 is uncommon.

However, it's a completely valid use case, and having to match against
`Err(NotFound)` is a pain, especially if you intend to use `try!`.
Keeping that in mind, `QueryResult` now has the `optional` method, which
converts `QueryResult<T>` to `QueryResult<Option<T>>`, treating
`NotFound` as `None`. Since [`result::Error` is not exhaustively
matchable](5435e30),
I do not think we need to change the error type in this method.

Since checking for and handling `NotFound` seems like a common case,
particularly for returning a 404 response in a web server, it has been
re-exported in the root namespace.

I was going to open a PR to get feedback on this change first, but after
making it, I feel the code quality improvement in our tests (especially
the doctests, where we've removed most `.unwrap()` calls) was enough to
convince me this is the right change.
@sgrif sgrif closed this as completed in caea99a Nov 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants