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

make genned table aliases more human-friendly #952

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@keredson
Contributor

keredson commented Jun 1, 2016

instead of: select t1.id from ledger_entry as t1
generate the alias from the table name: select le1.id from ledger_entry as le1

where the alias is an acronym of the table name. makes it easier to read large joins w/ many tables.

@coleifer unit tests not updated yet. i want to know if you're ok-with/opposed to this before i spend the time on that part.

@coleifer

This comment has been minimized.

Owner

coleifer commented Jun 1, 2016

I'm going to pass on this one as well. Seems like a special-case that really isn't that special. If you want to see an example of a custom query compiler, check out the tests -- there is one that will use the table name instead of t{\d}.

@coleifer coleifer closed this Jun 1, 2016

@keredson

This comment has been minimized.

Contributor

keredson commented Jun 1, 2016

this is just totally bizarre to me. why would you prefer meaningless table alias names? what possible benefit is that?

@coleifer

This comment has been minimized.

Owner

coleifer commented Jun 3, 2016

If you're concerned about table aliases, you can always specify a default to use with a given model. Again, this is documented.

class MyModel(Model):
    class Meta:
        table_alias = 'mm'

And @keredson tell your little buddies their emojis are really not helping anybody.

@coleifer

This comment has been minimized.

Owner

coleifer commented Jun 5, 2016

this is just totally bizarre to me. why would you prefer meaningless table alias names? what possible benefit is that?

Can you please dial down your outrage? Can you dial down your tone? It's not helpful.

@coleifer

This comment has been minimized.

Owner

coleifer commented May 17, 2017

For posterity, this is not a bad idea in that using the table's name to create the alias will make the SQL more easy to scan. It was for the following reasons I decided not to merge this:

  • Requires the addition of code to "fix" something that is technically not broken.
  • The purpose of Peewee is to abstract away the process of writing SQL.
  • You can explicitly set a model's table alias per-query or globally using existing APIs.
  • The implementation is (subjectively) gross.
  • Requires rewriting a huge number of test assertions.

Another misguided PR, not aligned with peewee's goals or aesthetic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment