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

Handle empty schema as AnyProperty instead of NoneProperty #417

Closed
wants to merge 6 commits into from
Closed

Handle empty schema as AnyProperty instead of NoneProperty #417

wants to merge 6 commits into from

Conversation

forest-benchling
Copy link
Collaborator

Closes #389.

@codecov
Copy link

codecov bot commented May 7, 2021 •
edited

Codecov Report

Merging #417 (68a61b8) into main (032a4a4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #417   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1549      1549           
=========================================
  Hits          1549      1549
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
openapi_python_client/parser/responses.py 100.00% <100.00%> (ø)

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 032a4a4...68a61b8. Read the comment docs.

forest-benchling
forest-benchling commented May 7, 2021
View reviewed changes
end_to_end_tests/golden-record/my_test_api_client/models/a_model.py
return one_of_models_type_1
except: # noqa: E722
pass
if not True:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know this generated is a little silly, but 🤷 it reduces the complexity of the JINJA templating

@forest-benchling forest-benchling changed the title Handle empty schema as AnyProperty Handle empty schema as AnyProperty instead of NoneProperty May 7, 2021
@forest-benchling forest-benchling marked this pull request as ready for review May 7, 2021 18:21
@forest-benchling
Copy link
Collaborator Author

Now ready for review @dbanty @emann

(cc @dtkav @GitOnUp @bowenwr)

@forest-benchling
Copy link
Collaborator Author

@dbanty I know you're probably busy, but any updates on when you might be able to take a look at this PR (and the other 2 I've submitted)?

@dbanty
Copy link
Collaborator

dbanty commented Jun 25, 2021

@forest-benchling I'm planning on knocking out a few more PRs this weekend, and probably will try to release 0.10.0 (which will include most of the work in progress) next weekend.

Can you resolve any conflicts to make that process a bit speedier?

@dbanty dbanty added this to the 0.10.0 milestone Jun 25, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2021 •
edited

Codecov Report

Merging #417 (7ae9ad1) into main (605c246) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #417   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1563      1563           
=========================================
  Hits          1563      1563
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
openapi_python_client/parser/responses.py 100.00% <100.00%> (ø)

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 605c246...7ae9ad1. Read the comment docs.

@forest-benchling
Copy link
Collaborator Author

@forest-benchling I'm planning on knocking out a few more PRs this weekend, and probably will try to release 0.10.0 (which will include most of the work in progress) next weekend.

Can you resolve any conflicts to make that process a bit speedier?

@dbanty Thanks. Done!

@dbanty dbanty mentioned this pull request Jul 2, 2021
Merged
@dbanty
Copy link
Collaborator

dbanty commented Jul 2, 2021

Looks great @forest-benchling , thanks! I am going to fix the template to not need that extra if, so I'll include your changes in #445 instead of merging this one.

@dbanty dbanty closed this Jul 2, 2021
dbanty added a commit that referenced this pull request Jul 2, 2021
@dbanty @forest-benchling
fix(generator): Treat empty schemas like Any instead of None. Tha…
0056677 
…nks @forest-benchling! [#417 & #445]

* Support AnyProperty

* Install Poetry requirements and re

* Remove NoneProperty

* Add test case, change to check for Any instead of None

* Fix test

* fix: Remove need for unnecessary `if True` when constructing unions of Any properties.

Co-authored-by: Forest Tong <forest@benchling.com>
@forest-benchling
Copy link
Collaborator Author

Thanks @dbanty. @GitOnUp @packyg this is the last piece we will need to fix the MyPy issues, by making Field have type Any.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.

Use Any for schemas with no type
3 participants
@forest-benchling @dbanty @codecov-commenter