[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
jon-evergreen commented on PR #28202: URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344578092 Also, in case it wasn't clear, I have now checked in the updated `sql.pyi` file -- 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
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
jon-evergreen commented on PR #28202: URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344327662 I did install the hooks after the first commit which got bounced. And when I make changes to the `sql.py` I get this in the pre-commit logs: ``` Check and update common.sql API stubs..(no files to check)Skipped ``` I think the precommit definition needs to change from ``` ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.py[i]$ ``` to ``` ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$ ``` to have it run on changes to both `sql.py` and `sql.pyi`, as the static checks action is doing. -- 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
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
jon-evergreen commented on PR #28202: URL: https://github.com/apache/airflow/pull/28202#issuecomment-1342547253 Ah, the static checks for BigQuery are failing because there's no longer a `self.sql` attribute. This suggests to me maybe I should keep `self.sql` around, and have that be templated -- 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
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
jon-evergreen commented on PR #28202: URL: https://github.com/apache/airflow/pull/28202#issuecomment-1342490499 Yes, sorry, I missed those yesterday. This code took the slightly more complex approach of moving things around such that the SQL gets generated only in the execute method. I did notice some of the operators take the approach of templating what would be `self.sql`, which allows templating basically everywhere in the SQL, and would also mean the full sql statement would appear in the "Rendered" tab in the UI, which could be nice. Though it also means you don't get the `templated_fields` highlighting that e.g. you can template the `partition_clause` argument. Though it would technically be more work for the task, I guess you could template all the things currently templated more for a documentation PoV _and_ the `sql` field which the operator generates itself; the extra compute time would be relatively small, I imagine? What do you think? -- 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
