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.

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

fix: set default to None for pydantic generator #1663

Merged
merged 2 commits into from
Dec 19, 2023
Merged

fix: set default to None for pydantic generator #1663

merged 2 commits into from
Dec 19, 2023

Conversation

Harshil-Jani
Copy link
Contributor

In pydantic based python generator, when the optional values are passed, they need to be passed with an default=None argument in Field().

image

Related issue(s)
Fixes #1644

@netlify Netlify
Copy link

netlify bot commented Dec 15, 2023 •
edited
Loading

✅ Deploy Preview for modelina canceled.

Name Link
🔨 Latest commit 9407c3f
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/6581dec001117700082b36b7

jonaslagoni
jonaslagoni requested changes Dec 16, 2023
View reviewed changes
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Got some build and test problems, should be an easy fix. Let me know if you need anything 😄

@Harshil-Jani
Copy link
Contributor Author

Got some build and test problems, should be an easy fix. Let me know if you need anything 😄

Oh I see, I was not aware about the tests. Just updated the snapshots to work and added in same commit. Thanks for helping.

@jonaslagoni
Copy link
Member

@Harshil-Jani remember to do the same for the examples ✌️

jonaslagoni
jonaslagoni requested changes Dec 18, 2023
View reviewed changes
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Still need to update the examples npm run test:examples:update 🙂

@Harshil-Jani
Copy link
Contributor Author

@jonaslagoni On it. Sorry for the delay. Will be fixing it right in next few hours.

@Harshil-Jani Harshil-Jani force-pushed the harshil/fix-pydantic branch 2 times, most recently from a6b524d to b519dee Compare December 19, 2023 09:11
jonaslagoni
jonaslagoni approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Nice 💪

@jonaslagoni
Copy link
Member

/rtm

@coveralls
Copy link

coveralls commented Dec 19, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 7265950504

  • 9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 92.494%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/generators/python/presets/Pydantic.ts 9 10 90.0%
Totals Coverage Status
Change from base Build 7265084992: -0.02%
Covered Lines: 5606
Relevant Lines: 5908
💛 - Coveralls

jonaslagoni
jonaslagoni requested changes Dec 19, 2023
View reviewed changes
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Now a few linting problems 😄

@Harshil-Jani
Copy link
Contributor Author

Now a few linting problems 😄

@jonaslagoni Is there a way I can test the run locally on my system for linting, examples, tests all using a single command ? This way, It would be a lot easier for you to maintain 😢. And also this would reduce the actions ran on the CI. Or if it is not possible with just one command then maybe having it on the PR checklist would ensure that new contributors actually run things locally and pass tests or linting errors.

@jonaslagoni
Copy link
Member

We actually dont 🤔
Great idea @Harshil-Jani! Would you like to add it in another PR?

@Harshil-Jani
Copy link
Contributor Author

We actually dont 🤔 Great idea @Harshil-Jani! Would you like to add it in another PR?

Sure. I will fix the linting errors and add the Checklist in yaml file for PRs.

@Harshil-Jani
fix: set default to None for optional fields
1643da1 
- fix in Pydantic Python Generator for optional values.
- default=None should be added in `Field()` if the property is optional.
- updated the test snapshot to compare the new default value argument.

Signed-off-by: Harshil Jani <harshiljani2002@gmail.com>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@jonaslagoni
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 8390f85 into asyncapi:master Dec 19, 2023
16 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.0.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jonaslagoni
Copy link
Member

@all-contributors please add @Harshil-Jani for code and test

@allcontributors allcontributors bot mentioned this pull request Dec 19, 2023
Merged
@allcontributors AllContributors
Copy link
Contributor

@jonaslagoni

I've put up a pull request to add @Harshil-Jani! 🎉

@Harshil-Jani Harshil-Jani mentioned this pull request Dec 21, 2023
Merged
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next.11 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonaslagoni jonaslagoni jonaslagoni approved these changes

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

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

@asyncapi-bot-eve asyncapi-bot-eve Awaiting requested review from asyncapi-bot-eve asyncapi-bot-eve is a code owner

Assignees
No one assigned
Labels
ready-to-merge released on @next released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PythonGenerator with Pydantic If a property is Optional not required the json expected a key None
4 participants
@Harshil-Jani @jonaslagoni @coveralls @asyncapi-bot