Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish merged PR #44270: URL: https://github.com/apache/airflow/pull/44270 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
pierrejeambrun commented on PR #44270: URL: https://github.com/apache/airflow/pull/44270#issuecomment-2498609637 statement is fine by me too :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1856934578 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: i hereby declare that we shall go with `statement`. if it's good enough for sqlalchemy, it's good enough for us. (see arg to `session.execute`.) as dictator, appointed by myself, i declare there shall also be no more debate on this 🤪 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1856945042 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: but just in the spirit of trolling notice also https://github.com/user-attachments/assets/1047fbbc-8418-4c54-8525-d673bf969e51";> and https://github.com/user-attachments/assets/17f1bf46-4418-472b-9337-81fc01932187";> another long-existing example noticed upon while updating to `statement` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1856934578 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: i hereby declare that we shall go with `statement` if it's good enough for sqlalchemy, it's good enough for us. (see arg to `session.execute`. as dictator, appointed by myself, i declare there shall also be no more debate on this 🤪 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
pierrejeambrun commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1856597273 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: Why not `select_statement`. I aggree if query is already used everywhere, I'm ok to not overcomplicate it and keep query here too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
potiuk commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854916437 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review 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`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
potiuk commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854916437 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: Naming is hard. I agree `query` is bad choice. How about `select_statement` as a name ? Sounds pretty obvious to me - I think `select_obj` is proably worse idea than `query`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854640976 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854640976 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: also, @ephraimbuddy i'm not sure what you mean. it beomes some kind of a "result" object after running through `scalars`. but, ok, if y'all hate `query`, we can do `statement`. let me know. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854547713 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: See what i mean? `query` is a colloquial term for "select statement" Used in many places already https://github.com/user-attachments/assets/b07bd878-ae75-425b-9bae-befddfca7a95";> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854536845 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: and yes, query is a method on session; but this of course does not collide with that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854536071 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854536071 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: yeah i'm aware of `Query` but i don't think there is a `query` free function. query is a common word for what you would pass here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
dstandish commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1854536845 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: query is a method on session; but this of course does not collide with that. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
pierrejeambrun commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1853476594 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
rawwar commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1853439935 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review Comment: [query](https://docs.sqlalchemy.org/en/14/orm/query.html#query-api) 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` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Replace var names `select`, `base_select`, `base_query` with `query` [airflow]
ephraimbuddy commented on code in PR #44270: URL: https://github.com/apache/airflow/pull/44270#discussion_r1853421043 ## airflow/api_fastapi/common/db/common.py: ## @@ -52,23 +52,21 @@ def your_route(session: Annotated[Session, Depends(get_session)]): yield session -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: Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
