Skip to content

Conversation

@eumiro
Copy link
Contributor

@eumiro eumiro commented Sep 5, 2023

No description provided.

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 removed area:providers provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:docker labels Sep 11, 2023
@eladkal eladkal merged commit f70c107 into apache:main Sep 11, 2023
@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: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.

6 participants