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 .first method to RecordCollections #43

Merged
merged 10 commits into from
Nov 22, 2016
Merged

add a .first method to RecordCollections #43

merged 10 commits into from
Nov 22, 2016

Conversation

chadwhitacre
Copy link
Contributor

It's a common use-case to want one and only one result from a query.

@chadwhitacre
Copy link
Contributor Author

Includes #42.

@chadwhitacre
Copy link
Contributor Author

Rebased to fix a broken test in the appropriate commit, previous head was dcb9e7a.

@chadwhitacre
Copy link
Contributor Author

499fd5b allows for passing in an exception as default and having that be raised for you. Not sure if you'll want that feature even if you want the rest of the PR.

@chadwhitacre chadwhitacre changed the title [WIP] add a .one method to RecordCollections add a .one method to RecordCollections Feb 13, 2016
@chadwhitacre
Copy link
Contributor Author

Ready for review, @kennethreitz. :)

@kennethreitz
Copy link
Owner

So, I have mixed feelings about this one.

Currently, you can get this behavior with rows[0] (get just the first row), or rows[0:1].all(as_dict=True).
I do see some API symmetry between one & all, however.

I tend to lean on the side of introducing as few methods as possible to an API, and adding only what is necessary. This does seem like a good nice-to-have, though.

I am torn.

def one(self, default=None, as_dict=False, as_ordereddict=False):
"""Returns a single record for the RecordCollection, or `default`."""

# Try to get a record, or return default.
Copy link
Owner

Choose a reason for hiding this comment

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

This could be simplified to simply use use

record = self[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5e60bb9.

@chadwhitacre
Copy link
Contributor Author

Rebased on master at dd44086. Previous head was 5e60bb9.

@chadwhitacre
Copy link
Contributor Author

Currently, you can get this behavior with rows[0] (get just the first row), or rows[0:1].all(as_dict=True).

Yes, but quite often it should be a bug if there's more than one result, and/or not a bug if there's less than one. The purpose of the one method is to provide this error checking and defaulting.

I am torn.

Well, it's here if you want it. :-)

@Brobin
Copy link
Contributor

Brobin commented Feb 23, 2016

Would first be a better name? It's a little more specific to which one you are retrieving, and similar to other DB/ORM Api's I've seen before.

user = db.query('select * from users where id=1').first()

I do agree that just doing rows[0] is already pretty easy, and it's not that difficult.

@chadwhitacre
Copy link
Contributor Author

first makes it sound like you're expecting more than one, and you just want the first one, in which case there's not much advantage over rows[0].

one suggests that for the query to return more than one result is a bug.

@Brobin
Copy link
Contributor

Brobin commented Feb 23, 2016

Ah, gotcha. I didn't realize you were checking if there was more than one record.

@chadwhitacre
Copy link
Contributor Author

And also defaulting if there's less than one. :)

@kennethreitz
Copy link
Owner

@Brobin that was what I thought too. That's one of the main reasons I'm not gung ho about adding this — I think it's confusing :)

RecordCollection.exactly_one() on the other hand, hahaha.

@Brobin
Copy link
Contributor

Brobin commented Feb 23, 2016

RecordSet.exactly_one_and_fail_if_there_are_more() 😜

@kennethreitz
Copy link
Owner

I think .first() is what I want, and would be nice to have. It would return None if none exists, I think.

this needs a rebase if we wish to proceed ;)

It's a common use-case to want one and only one result from a query.
This adds a .one method to RecordCollections that is parallel to .all.
@chadwhitacre
Copy link
Contributor Author

Rebased on master at 9e65833. Previous head was a49d1ff. Renaming to first ... :-)

@chadwhitacre
Copy link
Contributor Author

No CI? I will run tests locally ...

@chadwhitacre
Copy link
Contributor Author

85fbec7

(env) $ py.test tests/
============================================ test session starts =============================================
platform darwin -- Python 3.5.0, pytest-2.8.7, py-1.4.31, pluggy-0.3.1
rootdir: /Users/whit537, inifile: pytest.ini
collected 15 items 

tests/test_records.py .............
tests/test_transactions.py ..

========================================= 15 passed in 1.47 seconds ==========================================
(env) $

@chadwhitacre
Copy link
Contributor Author

3b7438f

(env) $ py.test tests/
============================================ test session starts =============================================
platform darwin -- Python 3.5.0, pytest-2.8.7, py-1.4.31, pluggy-0.3.1
rootdir: /Users/whit537, inifile: pytest.ini
collected 15 items 

tests/test_records.py .............
tests/test_transactions.py ..

========================================= 15 passed in 1.42 seconds ==========================================
(env) $

@chadwhitacre chadwhitacre changed the title add a .one method to RecordCollections add a .first method to RecordCollections Nov 22, 2016
@chadwhitacre
Copy link
Contributor Author

Alright, @kennethreitz, lemme know if you want a squash or further changes or whatevs. :-)

@kennethreitz kennethreitz merged commit 4038d9d into kennethreitz:master Nov 22, 2016
@kennethreitz
Copy link
Owner

✨🍰✨

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.

3 participants