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

Update model fields for consistent usage of Optional and default #6829

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Sep 14, 2022

Makes two readability and consistency changes to our models:

There should be no change in behavior here. This was performed with some crazy regular expressions then manual edits for edge cases. Please give it a second look.

The lack of explicit Optional types lead to issues in #6813 — this change should unblock that.

Example

The following:

class Model(BaseModel):
   foo: str = Field(None)

has changed to:

class Model(BaseModel):
   foo: Optional[str] = Field(default=None)

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a fix, feature, enhancement, docs, collections, or maintenance label categorizing the change.

@zanieb zanieb added the maintenance Chores and other work not directly related to the product label Sep 14, 2022
@netlify
Copy link

netlify bot commented Sep 14, 2022

Deploy Preview for prefect-orion ready!

Name Link
🔨 Latest commit fe745d0
🔍 Latest deploy log https://app.netlify.com/sites/prefect-orion/deploys/63234a00b09d9200091797d6
😎 Deploy Preview https://deploy-preview-6829--prefect-orion.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@zanieb zanieb marked this pull request as ready for review September 14, 2022 19:10
@zanieb zanieb requested review from chrisguidry and removed request for tpdorsey and cicdw September 14, 2022 19:10
Copy link
Contributor

@bunchesofdonald bunchesofdonald left a comment

Choose a reason for hiding this comment

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

Looks good! I was a bit cross-eyed by the end, but all I found a couple places where Optional didn't need to be imported.

README.md Outdated Show resolved Hide resolved
src/prefect/task_runners.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Pickett <chris.pickett@prefect.io>
@zanieb
Copy link
Contributor Author

zanieb commented Sep 15, 2022

Thanks! Yeah I got to a point where I'd made all these changes and then everything was failing because Optional wasn't available so I added it to literally every import of typing then relied on autoflake8 to drop it. A few places that doesn't go though :)

Copy link
Collaborator

@chrisguidry chrisguidry left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense to me, and the only change we might need to check in on with Prefect Cloud is the ORMBaseModel. I'm not expecting any trouble, what you've done here is totally compatible and just making the implicit explicit (thank you!)

@zanieb zanieb merged commit cff2191 into main Sep 15, 2022
@zanieb zanieb deleted the results/3-fix-models branch September 15, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Chores and other work not directly related to the product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants