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

desc silently fails if it doesn't handle the argument #646

Open
nburns opened this issue May 13, 2022 · 2 comments
Open

desc silently fails if it doesn't handle the argument #646

nburns opened this issue May 13, 2022 · 2 comments

Comments

@nburns
Copy link

nburns commented May 13, 2022

Found this strange behavior:
Descending (note the lack of a DESC in the output:

ipdb> select(t for t in ReplyTemplate).order_by(desc(lambda t: t.id)).get_sql()
'SELECT DISTINCT `t-1`.`id`, `t-1`.`enabled`, `t-1`.`company`, `t-1`.`kind`, `t-1`.`created_by`, `t-1`.`modified_by`, `t-1`.`title`, `t-1`.`body`, `t-1`.`display_order`, `t-1`.`time_created`, `t-1`.`time_modified`, `t-1`.`shadowed_system_template`\nFROM `reply_template` `t-1`\nORDER BY `t-1`.`id`'

Nothing (ascending)

ipdb> select(t for t in ReplyTemplate).order_by(lambda t: t.id).get_sql()
'SELECT DISTINCT `t-1`.`id`, `t-1`.`enabled`, `t-1`.`company`, `t-1`.`kind`, `t-1`.`created_by`, `t-1`.`modified_by`, `t-1`.`title`, `t-1`.`body`, `t-1`.`display_order`, `t-1`.`time_created`, `t-1`.`time_modified`, `t-1`.`shadowed_system_template`\nFROM `reply_template` `t-1`\nORDER BY `t-1`.`id`'

looking at the source of desc:

def desc(expr):
    if isinstance(expr, Attribute):
        return expr.desc
    if isinstance(expr, DescWrapper):
        return expr.attr
    if isinstance(expr, int_types):
        return -expr
    if isinstance(expr, basestring):
        return 'desc(%s)' % expr
    return expr

It looks like this is intended? Can you help me understand why it behaves like this?

@sashaaero
Copy link
Member

sashaaero commented May 15, 2022

Hello!
The correct usage in your case would be either

select(t for t in ReplyTemplate).order_by(lambda t: desc(t.id)).get_sql()

or

select(t for t in ReplyTemplate).order_by(desc(ReplyTemplate.id))

@nburns
Copy link
Author

nburns commented May 19, 2022

Thanks for taking a look and thanks for the response!

What do you think about something like raising if callable(expr)?

Also happy to do a PR just wanted to talk through it first so it wasn't a surprise or never got reviewed.

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