Skip to content

ASF: Mark optional params as such (X | None) #12614

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
merged 2 commits into from
May 13, 2025
Merged

ASF: Mark optional params as such (X | None) #12614

merged 2 commits into from
May 13, 2025

Conversation

bblommers
Copy link
Contributor

@bblommers bblommers commented May 13, 2025 •
edited
Loading

Motivation

As discussed in #12588 - if we want to enable MyPy on the entire codebase, one of the first steps would be to make sure that the generated API has the correct types.

Changes

  • Marks optional parameters as such (X | None = None instead of X = None)
  • Updates the test_asf_providers.py::test_provider_signatures to handle all variations of Optional/Union params
  • Updates the API itself

@bblommers bblommers added this to the Playground milestone May 13, 2025
@bblommers bblommers added area: asf semver: patch Non-breaking changes which can be included in patch releases labels May 13, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented May 13, 2025 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files  ±0    2 suites  ±0   8m 45s ⏱️ +32s
488 tests ±0  438 ✅ ±0   50 💤 ±0  0 ❌ ±0 
976 runs  ±0  876 ✅ ±0  100 💤 ±0  0 ❌ ±0 

Results for commit 218d504. ± Comparison against base commit 6910145.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 13, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files      2 suites   1h 42m 1s ⏱️
4 425 tests 4 042 ✅ 383 💤 0 ❌
4 427 runs  4 042 ✅ 385 💤 0 ❌

Results for commit 218d504.

♻️ This comment has been updated with latest results.

@bblommers bblommers marked this pull request as ready for review May 13, 2025 10:02
@bblommers bblommers requested a review from thrau as a code owner May 13, 2025 10:02
@bblommers bblommers requested a review from alexrashed May 13, 2025 10:02
alexrashed
alexrashed approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Small and concise change and directly as discussed in the other linked PRs / conversations!
Would it make sense to maybe also update the APIs directly here in the PR in a separate commit and in our downstream project right after merging this one, instead of mixing it with the weekly automatically scheduled execution of the update next Monday? :)

alexrashed
alexrashed approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

🤩

MEPalma
MEPalma approved these changes May 13, 2025
View reviewed changes
Copy link
Contributor

@MEPalma MEPalma left a comment

Choose a reason for hiding this comment

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

The extra annotation for SFN is definitely welcome, as is the possibility to use MyPy; thank you!

Just curious, are there plans/or would it be feasible for us to distinguish between Optional[X] and NotRequired[X]? I find that distinction quite useful.

@bblommers
Copy link
Contributor Author

Just curious, are there plans/or would it be feasible for us to distinguish between Optional[X] and NotRequired[X]? I find that distinction quite useful.

@MEPalma I don't think I've ever used NotRequired actually!

Do you find the distinction useful when invoking a method(arg: NotRequired) to better understand the signature, or is it easier during the implementation of that method? As far as I can tell, there is no practical difference for the implementation - so I'm inclined to stick to Optional, just because it's simpler to read/understand.

pinzon
pinzon reviewed May 13, 2025
View reviewed changes
Copy link
Member

@pinzon pinzon left a comment

Choose a reason for hiding this comment

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

👍

@bblommers bblommers merged commit 9b6ef35 into master May 13, 2025
41 checks passed
@bblommers bblommers deleted the asf-optional-params branch May 13, 2025 17:31
@bblommers bblommers mentioned this pull request May 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinzon pinzon pinzon left review comments

@alexrashed alexrashed alexrashed approved these changes

@silv-io silv-io silv-io approved these changes

@MEPalma MEPalma MEPalma approved these changes

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

@sannya-singal sannya-singal Awaiting requested review from sannya-singal sannya-singal is a code owner

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

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

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

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

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

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

@viren-nadkarni viren-nadkarni Awaiting requested review from viren-nadkarni viren-nadkarni is a code owner

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

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

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

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

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

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

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

Assignees
No one assigned
Labels
area: asf semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
Playground
Development

Successfully merging this pull request may close these issues.
5 participants
@bblommers @alexrashed @silv-io @pinzon @MEPalma