Skip to content

fix: Fix deserialization of non-required properties #334

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

Merged
merged 4 commits into from
Feb 10, 2021

Conversation

forest-benchling
Copy link
Collaborator

@forest-benchling forest-benchling commented Feb 10, 2021

Previously, a non-required datetime would generate code like this in the from_dict:

image (1)

The if statement is incorrect as it does not check for Unset, and furthermore the cast (which is best to avoid using when possible) masks the issue.

This had already been fixed for models in #315, but not for dates, datetimes, enums, or files.

In this PR, I pulled out the shared logic into a higher-order macros construct_template, in order to fix the issue across the board.

@forest-benchling
Copy link
Collaborator Author

cc @dbanty @emann @bowenwr @packyg

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #334 (bbde89a) into main (b2adc2c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #334   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1390      1390           
=========================================
  Hits          1390      1390           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2adc2c...bbde89a. Read the comment docs.

Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Awesome, this is a great refactor! Just one residual print to delete probably left from debugging.

This will likely conflict with #332 from what I've read, do you have a preference which gets merged first?

Comment on lines +27 to +28


Copy link
Collaborator

Choose a reason for hiding this comment

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

Odd, I'd think that black would have removed that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I couldn't figure out why that got added by my PR...

)

template = env.get_template("date_property_template.py")
content = template.render(property=prop)
print(content)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(content)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:eek: thanks

@forest-benchling
Copy link
Collaborator Author

Awesome, this is a great refactor! Just one residual print to delete probably left from debugging.

This will likely conflict with #332 from what I've read, do you have a preference which gets merged first?

@dbanty Yeah, I think we can merge this one first and I'll address the conflicts on #332, since it has more changes to be addressed

Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@dbanty dbanty merged commit 351478f into openapi-generators:main Feb 10, 2021
dbanty added a commit that referenced this pull request Feb 10, 2021
@forest-benchling forest-benchling deleted the forest-datetime branch February 10, 2021 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants