Skip to content

Comments

Update old style typing#26872

Merged
uranusjr merged 2 commits intoapache:mainfrom
pierrejeambrun:update-old-style-typing
Oct 27, 2022
Merged

Update old style typing#26872
uranusjr merged 2 commits intoapache:mainfrom
pierrejeambrun:update-old-style-typing

Conversation

@pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Oct 4, 2022 •
edited
Loading

Related: #26290

Try to remove as much as possible the few remaining old style typing:

  • Remove Dict, List, Set, etc... (everywhere possible) Cannot get 100% rid of it due to specific edge cases.
  • Remove :rtype: and make sure that return type annotation is specified accordingly
  • Remove comments type to use 'real' type annotation.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:plugins area:providers area:Scheduler including HA (high availability) scheduler provider:Apache provider:amazon AWS/Amazon - related issues provider:common-sql provider:databricks provider:google Google (including GCP) related issues labels Oct 4, 2022
potiuk
potiuk reviewed Oct 4, 2022
View reviewed changes
@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch from ce1fb5b to 4cce157 Compare October 4, 2022 22:26
@potiuk
Copy link
Member

potiuk commented Oct 4, 2022 •
edited
Loading

I think there are two reasons for missing conversions:

  1. .pyi files
  2. The # type: comments

for:

  1. I am not 100% sure how .pyi files work with future.annotations - but I guess we need the import :)
  2. the reason why pyubgrade did not convert them was that they were in comments aparently. I think we should convert it into "real" type hints from Python 3 - and I think it can even be done semi-automatically with smart regexp :D that will work in 9X% of cases.

potiuk
potiuk reviewed Oct 4, 2022
View reviewed changes
@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Oct 4, 2022 •
edited
Loading

CI is failing, 3.7 has some issue right on the test_infer_multiple_outputs_using_dict_typing, i'll look into that

@potiuk
Copy link
Member

potiuk commented Oct 4, 2022

Ah yeah there was one more place i left Dict as it was sactually used in tests (inference of serialization type based on runtime typing information) :) . Those are the failing tests I think

@uranusjr
Copy link
Member

uranusjr commented Oct 5, 2022

I think most of the :rtype: annotations should actually be removed outright, since the return type is already available in the signature and they are just duplication.

@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch from 4cce157 to a62ec31 Compare October 9, 2022 17:29
@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Oct 9, 2022 •
edited
Loading

Ok, indeed importing future annotations in the decorators/test_python.py, is breaking test_infer_multiple_outputs_using_dict_typing.

Leaving it like this.

@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch 3 times, most recently from 527f1a9 to 3f23714 Compare October 20, 2022 07:11
uranusjr
uranusjr reviewed Oct 20, 2022
View reviewed changes
uranusjr
uranusjr reviewed Oct 20, 2022
View reviewed changes
uranusjr
uranusjr reviewed Oct 20, 2022
View reviewed changes
uranusjr
uranusjr reviewed Oct 20, 2022
View reviewed changes
@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch 4 times, most recently from 0f9ade4 to a8feff0 Compare October 23, 2022 10:28
@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch from a8feff0 to 6a12873 Compare October 24, 2022 19:15
uranusjr
uranusjr reviewed Oct 25, 2022
View reviewed changes
uranusjr
uranusjr reviewed Oct 25, 2022
View reviewed changes
uranusjr
uranusjr approved these changes Oct 25, 2022
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

I think this is ready except for a couple of nitpicks above.

@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch 3 times, most recently from aa0fbce to 9c04e94 Compare October 25, 2022 22:21
potiuk
potiuk approved these changes Oct 26, 2022
View reviewed changes
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

One of the top contender for 'biggest number of files changed" PRs./

@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch from aad9465 to 956b62c Compare October 26, 2022 17:44
@pierrejeambrun
Update Optional and Dict types 1
e0c70a5 
Update Optional and Dict types 2

Update Optional and Dict types 3

Update Optional and Dict types 4

Update Tuple types

Update Set type

Update List type

Fix ci

Update missing return type

Update missing return 2

Remove rtype

Remove comment types

Fix doc building

Revist redundant :return: directives

Fix defaultdict typing.

Update following code review
@pierrejeambrun
Update following code review
4d68b03
@pierrejeambrun pierrejeambrun force-pushed the update-old-style-typing branch from 956b62c to 4d68b03 Compare October 26, 2022 22:45
@uranusjr uranusjr merged commit 9ab1a6a into apache:main Oct 27, 2022
@pierrejeambrun pierrejeambrun deleted the update-old-style-typing branch October 27, 2022 07:04
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Nov 16, 2022
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

@uranusjr uranusjr uranusjr approved these changes

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@jedcunningham jedcunningham Awaiting requested review from jedcunningham jedcunningham is a code owner

@turbaszek turbaszek Awaiting requested review from turbaszek

@dstandish dstandish Awaiting requested review from dstandish

@kaxil kaxil Awaiting requested review from kaxil kaxil is a code owner

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton

@bbovenzi bbovenzi Awaiting requested review from bbovenzi

@mik-laj mik-laj Awaiting requested review from mik-laj

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@XD-DENG XD-DENG Awaiting requested review from XD-DENG

@eladkal eladkal Awaiting requested review from eladkal

Assignees

No one assigned

Labels

area:plugins area:providers area:Scheduler including HA (high availability) scheduler full tests needed We need to run full set of tests for this PR to merge provider:amazon AWS/Amazon - related issues provider:cncf-kubernetes Kubernetes (k8s) provider related issues provider:common-sql provider:databricks provider:google Google (including GCP) related issues type:misc/internal Changelog: Misc changes that should appear in change log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pierrejeambrun @potiuk @uranusjr @ephraimbuddy