Skip to content
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.

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

Refactor: Consolidate 'import textwrap' in core #34113

Merged
merged 1 commit into from Sep 11, 2023

Conversation

eumiro
Copy link
Contributor

@eumiro eumiro commented Sep 5, 2023

No description provided.

@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools area:providers provider:cncf-kubernetes Kubernetes provider related issues provider:docker labels Sep 5, 2023
@eumiro eumiro marked this pull request as ready for review September 5, 2023 20:02
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

I'm a bit curious about why we want to do it this way. Also, would it make sense if we document these changes somewhere in the contribution document so that newcomers know what to follow?

@eumiro
Copy link
Contributor Author

eumiro commented Sep 6, 2023

I'm a bit curious about why we want to do it this way. Also, would it make sense if we document these changes somewhere in the contribution document so that newcomers know what to follow?

Grepping and counting for imports of textwrap shows:

     11 from textwrap import dedent
      3 from textwrap import wrap
     37 import textwrap

the majority imports textwrap directly, so I am suggesting to do it so everywhere.

@Lee-W
Copy link
Member

Lee-W commented Sep 7, 2023

I'm a bit curious about why we want to do it this way. Also, would it make sense if we document these changes somewhere in the contribution document so that newcomers know what to follow?

Grepping and counting for imports of textwrap shows:

     11 from textwrap import dedent
      3 from textwrap import wrap
     37 import textwrap

the majority imports textwrap directly, so I am suggesting to do it so everywhere.

Yep, sounds reasonable

@eladkal
Copy link
Contributor

eladkal commented Sep 8, 2023

Can you please split this PR to 2 PRs (and possibly all other refactoring):

  1. airflow/providers changes
  2. all the rest

it would make things easier for release managers as providers releases are separated from airflow core

@eladkal eladkal changed the title Refactor: Consolidate 'import textwrap' Refactor: Consolidate 'import textwrap' in core Sep 11, 2023
@eladkal eladkal merged commit f70c107 into apache:main Sep 11, 2023
64 checks passed
@eumiro eumiro deleted the import-textwrap branch September 11, 2023 17:53
@ephraimbuddy ephraimbuddy added this to the Airflow 2.7.2 milestone Oct 3, 2023
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Oct 3, 2023
@ephraimbuddy ephraimbuddy removed this from the Airflow 2.7.2 milestone Oct 4, 2023
@ephraimbuddy ephraimbuddy added this to the Airflow 2.8.0 milestone Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow area:dev-tools type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants