Refactor: Add type hints to utils.py and dbapi.py (#222) by YatesMold · Pull Request #256 · fedora-infra/tahrir-api

YatesMold · 2026-02-18T17:15:21Z

This PR addresses the first part of issue #222 by adding type hints to the core database API and utility functions.

Changes made:

Added typing annotations to tahrir_api/dbapi.py
Added typing annotations to tahrir_api/utils.py

All tests pass successfully locally via tox -e py310.

Fixes #222 (partially, as agreed with @gridhead to start from these core modules).

gridhead

@YatesMold, please look into the breaking format checks.

YatesMold · 2026-02-19T10:31:27Z

Thanks @gridhead! Good catch. I just ran black locally to format dbapi.py and utils.py, and pushed the update. The formatting checks should pass now. Let me know if everything looks good

gridhead

So far so good.

Here are some suggestions.

gridhead · 2026-02-24T03:46:58Z

tahrir_api/dbapi.py

        return series_id

-    def get_all_series(self):
+    def get_all_series(self) -> Any:


Please look into more specific types here instead of liberally using Any.

Maybe try doing this?

Suggested change

def get_all_series(self) -> Any:

def get_all_series(self) -> Query[Series]:

gridhead · 2026-02-24T03:47:07Z

tahrir_api/dbapi.py

        return unique_badges

-    def get_all_badges(self):
+    def get_all_badges(self) -> Any:


Please look into more specific types here instead of liberally using Any.

gridhead · 2026-02-24T03:47:19Z

tahrir_api/dbapi.py

        return self.get_milestone_from_badge_series(badge_id, series_id).count() != 0

-    def get_milestone_from_badge_series(self, badge_id, series_id):
+    def get_milestone_from_badge_series(self, badge_id: str, series_id: str) -> Any:


Please look into more specific types here instead of liberally using Any.

gridhead · 2026-02-24T03:48:43Z

tahrir_api/dbapi.py

        return unique_badges

-    def get_all_badges(self):
+    def get_all_badges(self) -> Any:


Maybe try doing this?

Suggested change

def get_all_badges(self) -> Any:

def get_all_badges(self) -> Query[Badge]:

gridhead · 2026-02-24T03:51:21Z

tahrir_api/dbapi.py

        return person.opt_out

-    def get_all_persons(self, include_opted_out=False):
+    def get_all_persons(self, include_opted_out: bool = False) -> Any:


Please look into more specific types here instead of liberally using Any.

gridhead · 2026-02-24T04:25:27Z

tahrir_api/dbapi.py

        :param expires_on: When this invitation expires.

-        :type created_by: str
+        :type created_by_email: str


Good work here too.

gridhead · 2026-02-24T04:26:10Z

tahrir_api/dbapi.py

+    ) -> str:
        """
        Add a new invitation to the database



Any reason why the remaining parameters are not included in the docstrings since you did make it a point to correct the ones that do exist?

Also, since the returns have been type-hinted too, along with those for the parameters, please include those too in your changes.

gridhead · 2026-02-24T04:29:51Z

tahrir_api/dbapi.py

+        person = self.get_person(created_by_email)
+        if not person:
+            raise ValueError(f"Could not retrieve person with email {created_by_email!r}")


What's with these paranoia-stricken double-checks when we did confirm before that the person exists? These repeated lookups against the database do not serve any practical purpose other than slowing down the performance of the API layer. Please explore your changes to remove such unnecessary changes.

gridhead · 2026-02-24T04:31:52Z

tahrir_api/dbapi.py

+        if not person:
+            return False


Good work here too.

gridhead · 2026-02-24T04:36:10Z

tahrir_api/dbapi.py

+        if not person:
+            return


Here again, we are adding the person if they do not exist, so there is no reason why we would not want to trust the execution of the function from a couple of lines above. Our database-accessing API is synchronous, and we would rather let these errors occur, as rare as they are, than have alarmingly many safety conditions throughout the codebase.

gridhead

See, we are open to AI-assisted contributions as long as the contributor follows the guidelines mentioned in the previously linked documentation. You have not been transparent about the usage of a large language model tooling into shaping your contributions, and your commit messages, 83af76c, 22c300c and 3cfe6a8, do not clearly describe that you have been assisted by a certain tooling.

Please look into this.

Assisted-by: Google Gemini

YatesMold · 2026-02-24T08:47:43Z

Hi @gridhead, thanks for the review and for pointing me to the docs! You are right. I used Google Gemini as a sounding board to double-check my type hint logic and to help draft the commit messages.

Since I'm still getting the hang of open source, I completely missed the AI contribution guidelines. My bad!
I'm doing an interactive rebase right now to add the Assisted-by: tags to my commit history. I'll force push the updates in a few minutes. Thanks for being patient with me

Assisted-by: Google Gemini

sdglitched

@YatesMold, it seems you have created a new commit for making the changes requested by @gridhead.

It's a good practice to keep the commits specific to the final changes rather than new commits for the changes requested by the reviewer.

Please squash the latest commit with the existing ones.

Also, the lint test seems to have failed. Please check and make necessary changes.

gridhead self-assigned this Feb 19, 2026

gridhead requested changes Feb 19, 2026

View reviewed changes

gridhead requested changes Feb 24, 2026

View reviewed changes

YatesMold added 3 commits February 24, 2026 10:38

Refactor: Add type hints to utils.py

9415a26

Assisted-by: Google Gemini

Refactor: Add type hints to dbapi.py

d21ae4e

Assisted-by: Google Gemini

Style: apply black formatting

f7e86b0

Assisted-by: Google Gemini

YatesMold force-pushed the issue-222-type-hints branch from 3cfe6a8 to f7e86b0 Compare February 24, 2026 08:44

Refactor: Address review feedback on dbapi.py type hints and performance

9115990

Assisted-by: Google Gemini

sdglitched suggested changes Feb 26, 2026

View reviewed changes

	def get_all_series(self) -> Any:
	def get_all_series(self) -> Query[Series]:

	def get_all_badges(self) -> Any:
	def get_all_badges(self) -> Query[Badge]:

Conversation

YatesMold commented Feb 18, 2026

Uh oh!

gridhead left a comment

Choose a reason for hiding this comment

Uh oh!

YatesMold commented Feb 19, 2026

Uh oh!

gridhead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gridhead left a comment

Choose a reason for hiding this comment

Uh oh!

YatesMold commented Feb 24, 2026

Uh oh!

sdglitched left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants