Skip to content

Clarify docstrings for updated DbApiHook #27966

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.

Sign up for GitHub

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

Jump to bottom
Merged
dstandish merged 2 commits into from
Nov 28, 2022
Merged

Clarify docstrings for updated DbApiHook #27966

dstandish merged 2 commits into from
Nov 28, 2022

Conversation

@dstandish
Copy link
Contributor

@dstandish dstandish commented Nov 28, 2022

Started off by fixing typo, replacing last_statement with return_last but then tried to clarify the language a bit here and there.

@dstandish dstandish requested a review from potiuk November 28, 2022 18:27
@dstandish dstandish requested a review from eladkal as a code owner November 28, 2022 18:27
dstandish
dstandish commented Nov 28, 2022
View reviewed changes
airflow/providers/common/sql/hooks/sql.py
Comment on lines -300 to -301
returned to XCom and generally fit in memory. As an optimization, handler is usually not executed
by the SQLExecuteQuery operator if `do_xcom_push` is not specified.
Copy link
Contributor Author

@dstandish dstandish Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this bit about sql execute operator, i thought isn't really relevant to hook.run docstring.

@potiuk
Copy link
Member

potiuk commented Nov 28, 2022

Line too long :(

@potiuk
Copy link
Member

potiuk commented Nov 28, 2022

Installed pre-commits really save on iterating with CI @dstandish 😛

dstandish
dstandish commented Nov 28, 2022
View reviewed changes
@dstandish
Copy link
Contributor Author

dstandish commented Nov 28, 2022

Installed pre-commits really save on iterating with CI @dstandish 😛

yeah, sigh... i have them enabled but i have flake8 and mypy disabled because....

iteration time 😎

just too slow on mac... although.... it was a while ago i last checked

@potiuk
Copy link
Member

potiuk commented Nov 28, 2022

Installed pre-commits really save on iterating with CI @dstandish 😛

yeah, sigh... i have them enabled but i have flake8 and mypy disabled because....

iteration time 😎

just too slow on mac... although.... it was a while ago i last checked

Let's speed'em up. On my M1 I have completely no problems with slow iteration time on commits :)

@potiuk
Copy link
Member

potiuk commented Nov 28, 2022

BTW. SKIP=run-mypy,run-flake8 as global env might help even if there are some slow-downs

@dstandish
Copy link
Contributor Author

dstandish commented Nov 28, 2022

BTW. SKIP=run-mypy,run-flake8 as global env might help even if there are some slow-downs

yes this is how i have it configured

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Nov 28, 2022

Haha I am familiar to this as well 😂.

What I do now is configure mypy and flake as linter in my IDE. This way it is really easy to see/resolve and iterate on typing error. Then the pre-commit hook are just an extra check :)

@dstandish dstandish merged commit a158fbb into apache:main Nov 28, 2022
@dstandish dstandish deleted the dbapi-hook-docstrings branch November 28, 2022 23:12
jrggggg pushed a commit to jrggggg/airflow that referenced this pull request Dec 1, 2022
@dstandish @jrggggg
Clarify docstrings for updated DbApiHook (apache#27966)
087ae3f
@eladkal eladkal mentioned this pull request Jan 2, 2023
Closed
84 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiuk potiuk potiuk approved these changes

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@eladkal eladkal Awaiting requested review from eladkal

Assignees

No one assigned

Labels

area:providers provider:common-sql

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dstandish @potiuk @pierrejeambrun