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

[ENHANCEMENT] Create a proxy for where in the form of get #84

Closed
coder3112 opened this issue Apr 5, 2021 · 7 comments
Closed

[ENHANCEMENT] Create a proxy for where in the form of get #84

coder3112 opened this issue Apr 5, 2021 · 7 comments

Comments

@coder3112
Copy link

Problem

Write now, the code I have to write becomes lengthy and hard to read if there are multiple conditions.

Example:

rows = await MyModel.select().where(Mymodel.property1 == property1).where(Mymodel.property2 == property2).where(Mymodel.property3 == property3).where(Mymodel.property4 >= property4).run()

The only thing I can do to make it better is:

rows. = await MyModel.select().where(
    MyModel.property1 == p1 &  MyModel.property2 == p2 &  MyModel.property3 == p3 & Mymodel.property4 >= p4
).run()

This doesn't look quite as intuitive and is super difficult to read in case of larger queries where there are multiple OR and AND at different complexities.

Solution I would like

A simple nice proxy to where in the form of get. (like save is to insert/update)

rows = await Mymodel.get(property1=p1, property2=p2, property3=p3, property4=p4).run()

Do you think this should be worked upon. Willing to submit a PR

@coder3112 coder3112 changed the title Create a proxy for where in the form of get [ENHANCEMENT] Create a proxy for where in the form of get Apr 5, 2021
@dantownsend
Copy link
Member

Yeah, it's a good idea. I think there are two options.

Modifying the where method

The where method could be modified, so it accepts multiple arguments.

# What's currently possible (all of these are equivalent):
await MyModel.select().where((MyModel.a == 1) & (MyModel.b == 2))
await MyModel.select().where(And(MyModel.a == 1, MyModel.b == 2))
await MyModel.select().where(MyModel.a == 1).where(MyModel.b == 2)

# What could be added:

# 1. Multiple arguments would behave the same as an AND, which does clean up the syntax a bit.
await MyModel.select().where(MyModel.a == 1, MyModel.b == 2)

# 2. Could also accept kwargs, which are converted to `MyModel.a == 1`.
await MyModel.select().where(a=1, b=2)

Adding a get method

I like the idea of adding get.

await MyModel.get(id=1)

# It's the equivalent of:
await MyModel.objects().where(MyModel.id == 1).first()

So it does save quite a bit of boiler plate code. I think this method would be for returning a single matching row, in the same way as MyModel.objects.get(id=1) in Django. What do you think?

@coder3112
Copy link
Author

coder3112 commented Apr 6, 2021

I think the 2nd method is best since it does not break existing functionality.
I think it could be simply implemented by doing something along the lines of:

@classmethod
def get(cls, **kwargs):
    parsed_args = # Parse the args
    return  Objects().where(AND(parsed_args))

@dantownsend
Copy link
Member

The first method could be implemented in a backwards compatible way. The where signature would just be changed from:

def where(self, where: Combinable):
    self.where_delegate.where(where)
    return self

To:

def where(self, *where: Combinable, **kwargs):
    for i in where:
        self.where_delegate.where(i)

    for key, value in kwargs.items():
         column = self._meta.get_column_by_name(key)
         self.where_delegate.where(Where(column=column, value=value, operator=Equal))

    return self

If implementing option 2, it would then just be:

@classmethod
def get(cls, *where: Combinable, **kwargs):
    return self.objects().where(*args, **kwargs).first()

What do you think?

@coder3112
Copy link
Author

coder3112 commented Apr 7, 2021

Hmmm, yes, I think this is exactly what we need although .first() can be potentially problematic. Why not just return the entire queryset?

@dantownsend
Copy link
Member

It's just to mimic how get works in Django, and also in Python in general.

For example:

>>> foo = {'a': 1}
>>> foo.get('a')
1

>>> foo.get('b')
None

If all results are needed, it could be queried like await MyModel.objects().where(a=1). Or maybe a method name other than get to avoid confusion.

@coder3112
Copy link
Author

coder3112 commented Apr 7, 2021

Okay, sounds great. Do you need a PR? Working on one right now.

Okay, modified #83 to accomodate this as well

@dantownsend
Copy link
Member

There are now get and get_or_create methods in master now - I'll close this for now, but feel free to reopen if you feel anything needs adding.

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

No branches or pull requests

2 participants