Skip to content
This repository has been archived by the owner on Sep 12, 2024. It is now read-only.

[FLAPI-2083] Fix various models type hints when maybe parsing #54

Merged
merged 8 commits into from
Jan 5, 2022

Conversation

jesse-c
Copy link
Contributor

@jesse-c jesse-c commented Jan 4, 2022

Fix Order model type hints

By (possibly) setting the value varaible to a non-iterable type, we get warnings. Instead, explicitly continue on once we maybe parse a str type. We wouldn't want to iterate over a str.

We don't serialise Payments for Orders so I've removed them from the appropriate fixtures.

TODO

  • Rebase fixup commits
  • Fix complexity warnings

@jesse-c jesse-c requested a review from a team January 4, 2022 16:15
@jesse-c jesse-c requested a review from a team as a code owner January 4, 2022 16:15
@jesse-c jesse-c requested a review from enginoid January 4, 2022 16:15
@jesse-c jesse-c self-assigned this Jan 4, 2022
@jesse-c jesse-c requested a review from sgerrand January 4, 2022 16:15
@jesse-c jesse-c force-pushed the fix-type-order-model-types branch 2 times, most recently from c8ed337 to d37cca5 Compare January 4, 2022 16:21
@jesse-c jesse-c changed the title Fix type hints on Order model Fix Order, Offer models type hints Jan 4, 2022
@jesse-c jesse-c force-pushed the fix-type-order-model-types branch from d37cca5 to 24b8e52 Compare January 4, 2022 16:24
@jesse-c jesse-c changed the title Fix Order, Offer models type hints Fix various models type hints when maybe parsing Jan 4, 2022
@jesse-c jesse-c force-pushed the fix-type-order-model-types branch from 24b8e52 to d351722 Compare January 4, 2022 16:28
@jesse-c jesse-c changed the title Fix various models type hints when maybe parsing [FLAPI-2040] Fix various models type hints when maybe parsing Jan 4, 2022
Copy link
Contributor

@sgerrand sgerrand left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

Copy link
Contributor

@sgerrand sgerrand left a comment

Choose a reason for hiding this comment

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

🤔

duffel_api/utils.py Show resolved Hide resolved
Copy link
Contributor

@sgerrand sgerrand left a comment

Choose a reason for hiding this comment

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

Some of these changes have broken the implementation – check the integration tests in examples/ for pointers to where.

@jesse-c
Copy link
Contributor Author

jesse-c commented Jan 5, 2022

@sgerrand: I'll go through them today.

@sgerrand
Copy link
Contributor

sgerrand commented Jan 5, 2022

Thanks Jesse!

@jesse-c jesse-c force-pushed the fix-type-order-model-types branch 2 times, most recently from 1091597 to 508de6f Compare January 5, 2022 14:16
@jesse-c jesse-c requested a review from sgerrand January 5, 2022 14:19
@sgerrand
Copy link
Contributor

sgerrand commented Jan 5, 2022

I'm pointing out the obvious but tox -e linting is unhappy.

  ./duffel_api/models/order.py:20:5: C901 'Order.__init__' is too complex (12)
16
      def __init__(self, json):
17
      ^
18
  ./duffel_api/models/offer.py:23:5: C901 'Offer.__init__' is too complex (12)
19
      def __init__(self, json):
20
      ^

@jesse-c
Copy link
Contributor Author

jesse-c commented Jan 5, 2022

Yup, I'm working on those atm. I've got them recorded in the TODO list in the description. Thanks for making sure it's not missed though!

I think I've found some pre-existing bugs through refactoring this. 😬

@sgerrand
Copy link
Contributor

sgerrand commented Jan 5, 2022

Nice – that's good thing!

@jesse-c jesse-c force-pushed the fix-type-order-model-types branch from f68be40 to e5e1d2c Compare January 5, 2022 15:20
@jesse-c
Copy link
Contributor Author

jesse-c commented Jan 5, 2022

@sgerrand: This is ready for review now.

Copy link
Contributor

@sgerrand sgerrand left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@jesse-c jesse-c force-pushed the fix-type-order-model-types branch from e5e1d2c to ca6005f Compare January 5, 2022 16:38
@jesse-c jesse-c enabled auto-merge January 5, 2022 16:38
@jesse-c jesse-c merged commit 9717979 into main Jan 5, 2022
@jesse-c jesse-c deleted the fix-type-order-model-types branch January 5, 2022 16:39
@jesse-c jesse-c changed the title [FLAPI-2040] Fix various models type hints when maybe parsing [FLAPI-2083] Fix various models type hints when maybe parsing Jan 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants