[FLAPI-2098] Add type annotations for models #79

jesse-c · 2022-01-13T11:25:18Z

The type annotations are added for all models. We use the dataclass to give us a consistent approach. We've added a from_json function specifically for that case.

I've chosen for now to not standardise the datetime again. If we discover a sane pattern to this, I'm happy to refactor it later.

Closes #81
Closes #82

sgerrand

It's probably a good moment to start adding unit tests for the model classes as well.

duffel_api/utils.py

jesse-c · 2022-01-13T12:22:56Z

@sgerrand: Specifically for the from_json functions?

sgerrand · 2022-01-13T12:56:25Z

Yes, please.

richcooper95 · 2022-01-14T14:47:08Z

duffel_api/models/order_cancellation.py

+            refund_currency=json["refund_currency"],
+            refund_to=json["refund_to"],
+            confirmed_at=parse_datetime(json["confirmed_at"]),
+            created_at=datetime.strptime(json["created_at"], "%Y-%m-%dT%H:%M:%S.%fZ"),


Is this meant to use the new parse_datetime function (below)?

It is not. That's for a value that requires for checking the length.

It feels like we may as well use it for consistency even if we don't actually need to check the length, since this case is handled in it - what do you think?

Just realised (d'oh) that the parse_datetime function is defined in this module! I think it'd be worth moving it to utils and using it every time we parse a datetime?

(Unless we want to explicitly error in the case where the precision isn't as expected for a given field?)

Not all date time values are the same format. There would need to be a key passed along to some shared function (like done previously), but I'm not a fan of that—or there could be n different parse date time value functions, which I'm also not a fan of.

I'll keep it as it is to avoid too early DRY'ing it out.

duffel_api/models/airport.py

richcooper95 · 2022-01-14T15:06:38Z

duffel_api/models/place.py

+            time_zone=json.get("time_zone"),
+            city_name=json.get("city_name"),
+            city=get_and_transform(json, "city", City.from_json),
+            airports=get_and_transform(


As above,

airports=get_and_transform( json, "airports", lambda value: [Airport.from_json(airport) for airport in value], ),

could become

airports=[Airport.from_json(a) for a in value] if "airports" in json else None

depending on which you prefer!

I'd need to do an equivalent for the line above (city=get_and_transform(json, "city", City.from_json),), which doesn't lend itself as nicely since it's not a list comprehension?

sgerrand

One early question, still reviewing!

duffel_api/models/airport.py

sgerrand · 2022-01-20T18:32:03Z

It looks like you're still actively pushing changes so I'll hold off for now and review this tomorrow.

jesse-c · 2022-01-20T18:32:41Z

@sgerrand: Sorry, yup! I've paused for today though, so it's safe. I'll do fixup commits tomorrow too to preserve the review comments.

jesse-c · 2022-01-20T18:36:53Z

duffel_api/http_client.py

@@ -61,7 +61,7 @@ def __iter__(self):
        while "meta" in response:
            after = response["meta"]["after"]
            for entry in response["data"]:
-                yield self._caller(entry)
+                yield self._caller.from_json(entry)


@richcooper95: I'm curious how we could ensure that the thing here being called has the from_json.

If self._caller is the class we want to instantiate, and we only want to allow self._caller to be set if it contains a from_json classmethod, then we could add validation* to the self._caller attribute - something along the lines of:

from inspect import getattr_static try: assert isinstance(getattr_static(x, "from_json"), classmethod) except (AttributeError, AssertionError) as _: raise TypeError("'caller' must have a 'from_json' classmethod") from None

*might be a good opportunity to try this method of validation?

@jesse-c: Did you have any thoughts on this?

I tried to reply by email, but it hasn't shown up!

Basically, I'm going to leave this for another PR, and have a proper think about it.

We need to refactor our classes to avoid circular dependencies.

jesse-c · 2022-01-28T18:20:55Z

I was going to worry about it in another PR since this once has become quite large.

…

On 28 Jan 2022, at 18:11, Rich Cooper ***@***.***> wrote: @richcooper95 commented on this pull request. In duffel_api/http_client.py: > @@ -61,7 +61,7 @@ def __iter__(self): while "meta" in response: after = response["meta"]["after"] for entry in response["data"]: - yield self._caller(entry) + yield self._caller.from_json(entry) @jesse-c: Did you have any thoughts on this? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

richcooper95

Some comments that might be worth thinking about while the hood's up, but it looks so good - huge improvement to user experience! ❤️

richcooper95 · 2022-01-28T18:14:42Z

duffel_api/api/booking/offer_requests.py

@@ -15,7 +15,9 @@ def __init__(self, **kwargs):

    def get(self, id_):
        """GET /air/offer_requests/:id"""
-        return OfferRequest(self.do_get("{}/{}".format(self._url, id_))["data"])
+        return OfferRequest.from_json(
+            self.do_get("{}/{}".format(self._url, id_))["data"]


Since we're probably going to move to f-strings over the codebase, may as well start here:

self.do_get(f"{self._url}/{id_}"))["data"]

I'll leave the f-strings for another PR!

richcooper95 · 2022-01-28T18:15:06Z

duffel_api/api/booking/order_cancellations.py

@@ -20,7 +20,9 @@ def __init__(self, **kwargs):

    def get(self, id_):
        """GET /air/order_cancellations/:id."""
-        return OrderCancellation(self.do_get("{}/{}".format(self._url, id_))["data"])
+        return OrderCancellation.from_json(
+            self.do_get("{}/{}".format(self._url, id_))["data"]


Ditto about f-strings

richcooper95 · 2022-01-28T18:15:15Z

duffel_api/api/booking/order_change_offers.py

@@ -11,7 +11,9 @@ def __init__(self, **kwargs):

    def get(self, id_):
        """GET /air/order_change_offers/:id"""
-        return OrderChangeOffer(self.do_get("{}/{}".format(self._url, id_))["data"])
+        return OrderChangeOffer.from_json(
+            self.do_get("{}/{}".format(self._url, id_))["data"]


richcooper95 · 2022-01-28T18:15:25Z

duffel_api/api/booking/order_change_requests.py

@@ -16,7 +16,9 @@ def create(self, order_id):

    def get(self, id_):
        """GET /air/order_change_requests/:id"""
-        return OrderChangeRequest(self.do_get("{}/{}".format(self._url, id_))["data"])
+        return OrderChangeRequest.from_json(
+            self.do_get("{}/{}".format(self._url, id_))["data"]


richcooper95 · 2022-01-28T18:15:35Z

duffel_api/api/booking/order_changes.py

@@ -26,7 +26,9 @@ def __init__(self, **kwargs):

    def get(self, id_):
        """GET /air/order_changes/:id."""
-        return OrderChange(self.do_get("{}/{}".format(self._url, id_))["data"])
+        return OrderChange.from_json(
+            self.do_get("{}/{}".format(self._url, id_))["data"]


richcooper95 · 2022-01-28T18:17:24Z

duffel_api/models/__init__.py

 from .airline import Airline
+from .airport import Airport, City, Place, Refund


Definitely want to change this - sorry I haven't got round to a better solution yet! I'll add an issue for it and assign it to myself so I don't forget!

richcooper95 · 2022-01-28T18:20:35Z

duffel_api/models/order_cancellation.py

+            refund_currency=json["refund_currency"],
+            refund_to=json["refund_to"],
+            confirmed_at=parse_datetime(json["confirmed_at"]),
+            created_at=datetime.strptime(json["created_at"], "%Y-%m-%dT%H:%M:%S.%fZ"),


It feels like we may as well use it for consistency even if we don't actually need to check the length, since this case is handled in it - what do you think?

richcooper95 · 2022-01-28T18:21:32Z

duffel_api/models/order_cancellation.py

+            refund_currency=json["refund_currency"],
+            refund_to=json["refund_to"],
+            confirmed_at=parse_datetime(json["confirmed_at"]),
+            created_at=datetime.strptime(json["created_at"], "%Y-%m-%dT%H:%M:%S.%fZ"),


Just realised (d'oh) that the parse_datetime function is defined in this module! I think it'd be worth moving it to utils and using it every time we parse a datetime?

richcooper95 · 2022-01-28T18:21:57Z

duffel_api/models/order_cancellation.py

+            refund_currency=json["refund_currency"],
+            refund_to=json["refund_to"],
+            confirmed_at=parse_datetime(json["confirmed_at"]),
+            created_at=datetime.strptime(json["created_at"], "%Y-%m-%dT%H:%M:%S.%fZ"),


(Unless we want to explicitly error in the case where the precision isn't as expected for a given field?)

richcooper95 · 2022-01-28T18:23:46Z

duffel_api/models/order_change_offer.py


-OrderChangeOfferSlicesRemove = OrderChangeOfferSlicesAdd
+def parse_datetime(value: str) -> datetime:


See comment above about defining this in utils and using everywhere!

This is a follow-on from previous changes (e.g. #79). I used Comby [1] for most of the changes. [1] `$ comby -i '"{}/{}:[rest]".format(:[arg1], :[arg2])' 'f"{:[arg1]}/{:[arg2]}:[rest]"' .py`

jesse-c changed the title ~~Add type annotations~~ Add type annotations for models Jan 13, 2022

jesse-c force-pushed the add-type-annotations branch 6 times, most recently from e9c8acd to b8e630b Compare January 13, 2022 11:39

sgerrand reviewed Jan 13, 2022

View reviewed changes

jesse-c force-pushed the add-type-annotations branch from b8e630b to 8b441a1 Compare January 13, 2022 12:09

jesse-c commented Jan 13, 2022

View reviewed changes

duffel_api/utils.py Outdated Show resolved Hide resolved

jesse-c force-pushed the add-type-annotations branch 3 times, most recently from da0c8c8 to 24b596b Compare January 13, 2022 12:21

jesse-c force-pushed the add-type-annotations branch 9 times, most recently from 47a9297 to 7e53c46 Compare January 13, 2022 17:10

richcooper95 reviewed Jan 14, 2022

View reviewed changes

duffel_api/models/airport.py Show resolved Hide resolved

richcooper95 reviewed Jan 14, 2022

View reviewed changes

jesse-c force-pushed the add-type-annotations branch 3 times, most recently from 38281bc to efb3844 Compare January 17, 2022 15:21

jesse-c force-pushed the add-type-annotations branch 2 times, most recently from a91b2c5 to 7ac54f7 Compare January 20, 2022 14:00

jesse-c marked this pull request as ready for review January 20, 2022 14:55

jesse-c requested a review from a team January 20, 2022 14:55

jesse-c requested a review from a team as a code owner January 20, 2022 14:55

jesse-c requested a review from sgerrand January 20, 2022 14:55

jesse-c force-pushed the add-type-annotations branch 5 times, most recently from 2c74a93 to 59f28ab Compare January 20, 2022 18:28

sgerrand reviewed Jan 20, 2022

View reviewed changes

duffel_api/models/airport.py Show resolved Hide resolved

jesse-c commented Jan 20, 2022

View reviewed changes

jesse-c force-pushed the add-type-annotations branch from d1a8133 to ffcf61e Compare January 24, 2022 11:08

jesse-c requested review from richcooper95 and sgerrand January 25, 2022 11:54

jesse-c mentioned this pull request Jan 25, 2022

Correct variable use in test #85

Merged

Jesse Claven added 3 commits January 25, 2022 13:11

Add type annotations for utils

0703f20

Add type annotations for models

85a902c

Re-combine (some) classes into single file

7fe1271

We need to refactor our classes to avoid circular dependencies.

jesse-c force-pushed the add-type-annotations branch from ffcf61e to 7fe1271 Compare January 25, 2022 13:12

richcooper95 approved these changes Jan 28, 2022

View reviewed changes

jesse-c merged commit 907d4a8 into main Jan 31, 2022

jesse-c deleted the add-type-annotations branch January 31, 2022 11:36

jesse-c mentioned this pull request Jan 31, 2022

[FLAPI-2162] Use f-strings everywhere #89

Merged

jesse-c changed the title ~~Add type annotations for models~~ [FLAPI-2098] Add type annotations for models Jan 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLAPI-2098] Add type annotations for models #79

[FLAPI-2098] Add type annotations for models #79

jesse-c commented Jan 13, 2022 •

edited

Loading

sgerrand left a comment

jesse-c commented Jan 13, 2022

sgerrand commented Jan 13, 2022

richcooper95 Jan 14, 2022

jesse-c Jan 17, 2022 •

edited

Loading

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

jesse-c Jan 31, 2022

richcooper95 Jan 14, 2022

jesse-c Jan 17, 2022

sgerrand left a comment

sgerrand commented Jan 20, 2022

jesse-c commented Jan 20, 2022

jesse-c Jan 20, 2022

richcooper95 Jan 21, 2022

richcooper95 Jan 28, 2022 •

edited

Loading

jesse-c Jan 31, 2022

jesse-c commented Jan 28, 2022 via email

richcooper95 left a comment

richcooper95 Jan 28, 2022

jesse-c Jan 31, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

richcooper95 Jan 28, 2022

		from .airline import Airline
		from .airport import Airport, City, Place, Refund


		OrderChangeOfferSlicesRemove = OrderChangeOfferSlicesAdd
		def parse_datetime(value: str) -> datetime:

[FLAPI-2098] Add type annotations for models #79

[FLAPI-2098] Add type annotations for models #79

Conversation

jesse-c commented Jan 13, 2022 • edited Loading

sgerrand left a comment

Choose a reason for hiding this comment

jesse-c commented Jan 13, 2022

sgerrand commented Jan 13, 2022

Choose a reason for hiding this comment

jesse-c Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgerrand left a comment

Choose a reason for hiding this comment

sgerrand commented Jan 20, 2022

jesse-c commented Jan 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richcooper95 Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesse-c commented Jan 28, 2022 via email

richcooper95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesse-c commented Jan 13, 2022 •

edited

Loading

jesse-c Jan 17, 2022 •

edited

Loading

richcooper95 Jan 28, 2022 •

edited

Loading