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

fix _get_type - check for NoneType #254

Merged
merged 5 commits into from
Sep 6, 2023
Merged

fix _get_type - check for NoneType #254

merged 5 commits into from
Sep 6, 2023

Conversation

dantownsend
Copy link
Member

Related to #245 (comment)

@dantownsend dantownsend added the bug Something isn't working label Sep 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 6, 2023

Codecov Report

❗ No coverage uploaded for pull request base (v1@92f8b2f). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

@@          Coverage Diff          @@
##             v1     #254   +/-   ##
=====================================
  Coverage      ?   92.34%           
=====================================
  Files         ?       33           
  Lines         ?     2039           
  Branches      ?        0           
=====================================
  Hits          ?     1883           
  Misses        ?      156           
  Partials      ?        0           

Copy link
Member

@sinisaos sinisaos left a comment

Choose a reason for hiding this comment

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

@dantownsend I can confirm that these changes are working as they should.

@dantownsend
Copy link
Member Author

@sinisaos Thanks for checking 👍

Just added one more test, but seems to failing.

@sinisaos
Copy link
Member

sinisaos commented Sep 6, 2023

@dantownsend Just an idea. If I use types.UnionType (because I think that Python 3.10 and above does not understand typing.Union) and modify the if statement to this

import types

if origin is t.Union or origin is types.UnionType:
    # same as before

the tests pass.

@dantownsend
Copy link
Member Author

@sinisaos Ah, great catch! I'll change it.

piccolo_api/fastapi/endpoints.py Dismissed Show dismissed Hide dismissed
@dantownsend dantownsend merged commit ecfc11b into v1 Sep 6, 2023
11 checks passed
dantownsend added a commit that referenced this pull request Oct 20, 2023
* pydantic v2 support (#245)

* pydantic_v2_support

* pin to Piccolo v1

* add comment

* remove KeyError Exception

* upgrade coverage

* fix type warnings with `nested`

* fix mypy warnings

* update black

* replacement for `outer_type`

* change assertion

---------

Co-authored-by: Daniel Townsend <dan@dantownsend.co.uk>

* update github actions

* bumped version

* fix `_get_type` - check for `NoneType` (#254)

* fix `_get_type` - check for `NoneType`

* add tests

* fix typo

* add a test for the new union syntax

* also check for `UnionType`

* bumped version

* update JSON schema (#257)

* bumped version

* Update requirements.txt

---------

Co-authored-by: sinisaos <sinisaos@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants