Skip to content

Refactor: Simplify code in smaller providers#33234

Merged
potiuk merged 1 commit into
apache:mainfrom
eumiro:providers-misc
Aug 20, 2023
Merged

Refactor: Simplify code in smaller providers#33234
potiuk merged 1 commit into
apache:mainfrom
eumiro:providers-misc

Conversation

@eumiro

@eumiro eumiro commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

Encompasses several providers. Can be split further to smaller PRs if needed.

Comment thread airflow/providers/daskexecutor/executors/dask_executor.py Outdated
Comment thread airflow/providers/http/hooks/http.py Outdated
Comment thread airflow/providers/http/hooks/http.py Outdated
@eumiro
eumiro force-pushed the providers-misc branch 8 times, most recently from 8c58906 to 890b3fb Compare August 12, 2023 19:18
Comment thread airflow/providers/common/sql/operators/sql.py Outdated
Comment thread airflow/providers/http/hooks/http.py Outdated
Comment thread airflow/providers/http/hooks/http.py Outdated
Comment thread airflow/providers/microsoft/azure/operators/batch.py Outdated

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need a rebase and to fix the static check errors

@eumiro

eumiro commented Aug 16, 2023

Copy link
Copy Markdown
Contributor Author

@uranusjr it looks like ruff does not like a multi-line abstract in a docstring introduced in:

8775f84

Wondering how that could get into the repo yesterday.

@uranusjr

Copy link
Copy Markdown
Member

@potiuk

potiuk commented Aug 19, 2023

Copy link
Copy Markdown
Member

Wondering how that could get into the repo yesterday.
See dicussion in https://apache-airflow.slack.com/archives/CCPRP7943/p1692077140945959

Yes. Just to add a bit.

Airflow is very much alive and changing. All the time. And it's very much alive, which means it's very much changing.

And it means that some expectations might change mid-flight for your PR. This is why we recommend everyone to rebase-early-rebase-often - also because we are adding and modifying static checks of ours continuously and it means that users who are rebasing stuff have a chance to see that there are new expectations (there is no chance a) everyone will read all the announcements on devluist / slack b) there is no other way to introduce changes, but to well, introduce them.

We have ~ 100-140 open PRs at any given time. Last week we merged 120 PRs by 51 authors. The chance that someon's merged change wil break your PR in-progress are pretty high. This is why I recommend ABR to everyone (Always Be Rebased). Pretty much every, single time you come-back to your PR, first thing to do is to rebase it on top of the latest main.

And yes. It might mean that sometimes your PR will have to be rebased 10 times and conflict resolved/new expectations added - because of someone else's change. That's a reality we are living in. And we have no choice but embrace it, be patient and continue rebasing and resolving conflicts/failed tests.

This is also one of the reasons I recommended to split the "huge" PR you had initially into many smaller ones. Because pretty much every time you'd rebase it, you would hit another few problems resulting from other's changes being merged in,

@potiuk

potiuk commented Aug 19, 2023

Copy link
Copy Markdown
Member

BTW. Sometimes even it happens that if someone did not rebase early and often and we merge it accidentally we have to revert it and ask the author to redo it, when it breaks something in our "main" - does not happen all the time, but it happens occassionally. so generally "Rebase-early, rebase-often" is a way to save rework and effort for everyone.

@potiuk
potiuk merged commit a91ee7a into apache:main Aug 20, 2023
@eumiro
eumiro deleted the providers-misc branch August 20, 2023 18:21
ephraimbuddy pushed a commit that referenced this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants