Replace var names select, base_select, base_query with query
#44270
Conversation
dstandish
requested review from
ephraimbuddy and
pierrejeambrun
as code owners
| def apply_filters_to_select( | ||
| *, base_select: Select, filters: Sequence[BaseParam | None] | None = None | ||
| ) -> Select: | ||
| def apply_filters_to_select(*, query: Select, filters: Sequence[BaseParam | None] | None = None) -> Select: |
There was a problem hiding this comment.
What do you think about select_obj since this is not yet a query? It becomes a query when we use session.scalars, session.execute etc.
There was a problem hiding this comment.
query is also a sqlalchemy function. But, I think, we can just leave it to be select cause, within the function, there's no need for sqlalchemy's select. If it's confusing, we can also make it _select or select_obj
There was a problem hiding this comment.
I vote for select_obj too. select and query are both in the SQLAchemy namespace. Also as Ephraim mentioned a Select is not the same as a Query.
+1 for removing the base in every name.
There was a problem hiding this comment.
yeah i'm aware of Query but i don't think there is a query free function. query is a very common word for what you would pass here. you can see it in very many places in the codebase where we use query to refer to a select.
There was a problem hiding this comment.
and yes, query is a method on session; but this of course does not collide with that.
There was a problem hiding this comment.
See what i mean?
query is a colloquial term for "select statement"
Used in many places already
There was a problem hiding this comment.
also, @ephraimbuddy i'm not sure what you mean. it beomes some kind of a "result" object after running through scalars. i'm not sure it every becomes a Query object. and anyway, again, i'm not naming the object because of its class (we have annotations to tell us that anyway) i am just giving it a name to describe what it is in simple terms.
but, ok, if y'all really hate query, we can do statement. please let me know. for now i leave it query.
There was a problem hiding this comment.
Naming is hard. I agree query is bad choice.
How about select_statement as a name ? Sounds pretty obvious to me - BTW. I think select_obj is proably worse idea than query.
Previosly it was `base_select` then recently renamed to `select`. `select` is not a great choice because it collides with the sqlalchemy function. Query is a better name. Also best to remove the "base" part of it because we tend to mutate it, making it not a "base" of anything.
dstandish
force-pushed
the
standardize-language-to-query
branch
from
9e15db0
to
9f3d067
Compare
Previosly it was
base_selectthen recently renamed toselect.selectis not a great choice because it collides with the sqlalchemy function. Query is a better name.Also best to remove the "base" part of it because we tend to mutate it, making it not a "base" of anything.