Add type safety and type hinting #317

ashleyzhang01 · 2024-11-05T22:34:20Z

Type safety is important for catching bugs early, facilitating refactoring, and improving overall maintainability.
We should use type hints makes the code more understandable to both developers and static type checkers, so our code can be more reliable and predictable

We should:

Use type hints
Write code in a more functional style
Add comments for things like magic values, unintuitive behaviors, gotchas, external context, etc.
Types should have capitalized names
Keep things pythonic (e.g. use list comprehensions, avoid explicit for-loops, use built-in functions and libraries)
Use type aliases to give semantic meaning to primitive types (e.g., ModelPath = str)
Functions should typically return a singular type rather than unions
Functions not used outside their module should be prefixed with _
Use context managers (with statements) for resource management
Consider using Protocol classes for structural typing
Use assert to help type checking when function logic limits return types
Use typing.cast as a no-op to inform the type checker of a type when needed
Use schemas instead of args and kwargs 😭 (and to replace Anys)

codecov · 2024-11-05T22:48:16Z

Codecov Report

Attention: Patch coverage is 94.57112% with 50 lines in your changes missing coverage. Please review.

Project coverage is 90.91%. Comparing base (44965e9) to head (5232436).

Files with missing lines	Patch %	Lines
backend/sublet/views.py	86.90%	11 Missing ⚠️
backend/pennmobile/admin.py	50.00%	7 Missing ⚠️
backend/portal/permissions.py	84.21%	6 Missing ⚠️
backend/sublet/serializers.py	90.90%	5 Missing ⚠️
...nndata/management/commands/get_fitness_snapshot.py	63.63%	4 Missing ⚠️
backend/gsr_booking/admin.py	70.00%	3 Missing ⚠️
backend/gsr_booking/models.py	94.91%	3 Missing ⚠️
backend/gsr_booking/api_wrapper.py	96.55%	2 Missing ⚠️
backend/utils/email.py	81.81%	2 Missing ⚠️
backend/dining/api_wrapper.py	90.90%	1 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   90.83%   90.91%   +0.08%     
==========================================
  Files          63       64       +1     
  Lines        2629     2851     +222     
==========================================
+ Hits         2388     2592     +204     
- Misses        241      259      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Clue88 · 2024-11-07T20:33:27Z

vcai122

Thanks for doing this!!! I have not worked with type systems in python before so a lot of it is just questions and my initial thoughts. PLEASE feel free to tell me I'm completely wrong since I assume you've been working with this stuff a lot :).

I am pretty opinionated about code though and I think its really important that we have a really clear story of how we want to use types, so here's my thoughts so far

Obviously types are an overall plus! Especially I really liked the parts that I saw where we had mostly pure python: api_wrappers, management commands, helper functions (types on get_usage help a lot)
I am scared of the Anys. I think ideally code with Anys should be rewritten/thought about so we can clean these up, but if we just ship it with Anys, we remove an incentive to fix it. BUT
- a lot are on dicts where its a Django dict which seems better (and we can fix it if we use some sort Django type library???)
- Tuneer suggested we can just have a rule where moving forward ppl are not allowed Anys and any code changes touching Any code must be fixed.
- A lot of it is on pretty bad code that needs to be rewritten anyway
I am concerned about being too "ad-hoc" about this. I'm talking about the Django part of the code here. I think if we want types to actually be helpful, and not just extra developer work without the benefits, we should be deliberate about the way we are adding types . Because Django comes with so much pre-built stuff and its so opinionated, we should consult what other people have done. To that end I've done some initial research:
- https://github.com/typeddjango/djangorestframework-stubs/tree/master/rest_framework-stubs seems promising but idk how to use it
- I think we need some examples of big production Django codebases that uses type safety we can model ours off of
- If we have to write our own types, we should be deliberate and put it in its own library

Let me know what you think

vcai122 · 2024-11-10T02:40:05Z

backend/dining/api_wrapper.py

        self.expiration = timezone.localtime() + datetime.timedelta(seconds=response["expires_in"])
        self.token = response["access_token"]

-    def request(self, *args, **kwargs):
+    def request(self, *args: Any, **kwargs: Any) -> requests.Response:


I guess we would want schemas here? Ideally there are built in ones since these are methods we just override?

vcai122 · 2024-11-10T02:41:33Z

backend/gsr_booking/api_wrapper.py

 from gsr_booking.serializers import GSRBookingSerializer, GSRSerializer
 from utils.errors import APIError


-User = get_user_model()
+if TYPE_CHECKING:


I'm not very familiar with how type checking works, but why do we have these weird versions of User?

I'm going to guess its because Django makes us do get_user_model()?

I feel like its clunky to have to put this at the top of all the files.. Maybe we can put this in a file and just import it everywhere? I'm starting to wonder how other Django projects do this cleanly

it's cause get_user_model() is a function, so it gets annoyed when we try to declare its type. but yes put it in another file under utils/types

backend/gsr_booking/views.py

vcai122 · 2024-11-10T03:20:28Z

backend/penndata/serializers.py

@@ -10,6 +12,9 @@
 )


+ValidatedData: TypeAlias = dict[str, Any]


I feel like we don't want this type alias here (also I don't think you did above). Instead, if we are going to have this alias, we should have it in some common shared type file (same with the weird User type thing we are doing).

But I think again, I'm wondering if we should be doing this "ad-hoc"-ly ourselves, and what existing large production type-safe Django projects do?

vcai122 · 2024-11-10T03:22:40Z

backend/penndata/views.py

+ValidatedData: TypeAlias = dict[str, Any]
+CalendarEventList: TypeAlias = QuerySet[CalendarEvent, Manager[CalendarEvent]]
+EventList: TypeAlias = QuerySet[Event, Manager[Event]]
+HomePageOrderList: TypeAlias = QuerySet[HomePageOrder, Manager[HomePageOrder]]


Same with these. I know they are for the sake of clarity, but I feel like its a bit overkill so just adding clutter here. And also in Django world, seeing that it is a queryset is actually probably more helpful.

vcai122 · 2024-11-10T03:26:09Z

backend/pennmobile/admin.py

-def add_post_poll_message(request, model):
+ModelType: TypeAlias = Type[Model]
+AdminContext: TypeAlias = Dict[str, Any]
+MessageText: TypeAlias = str


Same here, ifl ModelType should be in some common library (or better, installed library someone else has already put together). AdminContext would also benefit from coming from a common library (maybe more generically as Context as this is a common Django idea, or again better from some outside library). MessageText seems overkill.

vcai122 · 2024-11-10T03:28:03Z

backend/sublet/serializers.py

@@ -162,13 +169,19 @@ class Meta:
            "images",
        ]



Is there any chance serializers are not returning dictionaries?

if this is about the to_representation thing, it seems to be built into rest_framework to convert

vcai122 · 2024-11-10T03:31:26Z

backend/sublet/views.py

        queryset = self.get_queryset()
        filter = {"user": self.request.user.id, "sublet": int(self.kwargs["sublet_id"])}
-        obj = get_object_or_404(queryset, **filter)
+        obj: Offer = get_object_or_404(queryset, **filter)


What dictates if you explicitly annotate the type or not?

If the type of a variable cant be easily inferred (like x=5 is obv an int) or is ambiguous (None or type can change), it might require an explicit annotation. Or some packages we use may not use type annotations, so when we call those functions, we may need to specify. Or, sometimes types may be ambiguous, like ints and floats are often convertible or strs and datetimes if in right format, so you might want to declare type to increase guardrailing

* 🌷 add mypy * 🌌 fix mypy errors in portal * 👥user and sublet typing * 🍽️ fix dining typing * 🏘️ fix subletting tests typing * 📊 penndata typing errors * 📚 gsr booking type fixes * 🧎‍♀️OMFG NO WAY NO MORE TYPE ERRORS * 🧹 clean up types files * 💯 fix tests * 🌉 extend pre-commit rules

ashleyzhang01 added 7 commits November 4, 2024 20:45

💂 type safety and type hinting

4950ade

💄 beautify list properties filtering

7bf5144

🌷 add type safety and type hinting to portal

fc30c55

📚 add type safety and type hinting to pmobile

2264a3b

💥 add type safety and type checking everywehre

676b65a

🐍 make pollserializer more pythonic

1d2d912

🚀 add mypy to pre-commit

3f5fd46

ashleyzhang01 added 3 commits November 7, 2024 11:58

💔 take out mypy for now

0df3281

🤖 add more typing

4d69e42

🧩 Merge master into add-type-safety

848c72b

ashleyzhang01 marked this pull request as ready for review November 7, 2024 20:12

ashleyzhang01 requested review from dr-Jess, vcai122, tuneerroy and judtinzhang November 7, 2024 20:14

vcai122 reviewed Nov 10, 2024

View reviewed changes

ashleyzhang01 and others added 4 commits November 13, 2024 16:16

Merge branch 'master' into add-type-safety

9e709f5

🧹 clean up type aliases

c0bffdb

🚿 clean up type aliases more

5232436

ashleyzhang01 requested a review from vcai122 November 14, 2024 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type safety and type hinting #317

Add type safety and type hinting #317

ashleyzhang01 commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading

Clue88 commented Nov 7, 2024

vcai122 left a comment

vcai122 Nov 10, 2024

vcai122 Nov 10, 2024

vcai122 Nov 10, 2024

ashleyzhang01 Nov 14, 2024

vcai122 Nov 10, 2024

vcai122 Nov 10, 2024

vcai122 Nov 10, 2024

vcai122 Nov 10, 2024

ashleyzhang01 Nov 14, 2024

vcai122 Nov 10, 2024

ashleyzhang01 Nov 14, 2024

Add type safety and type hinting #317

Are you sure you want to change the base?

Add type safety and type hinting #317

Conversation

ashleyzhang01 commented Nov 5, 2024 • edited Loading

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

Clue88 commented Nov 7, 2024

vcai122 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

ashleyzhang01 commented Nov 5, 2024 •

edited

Loading

codecov bot commented Nov 5, 2024 •

edited

Loading