Skip to content

Fix type hints for enums referring to ttypes #971

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

earangol-stripe
Copy link
Contributor

@earangol-stripe earangol-stripe commented Apr 15, 2025

Summary

Changes type hints for enum values to ints instead of the class type.

Why / Goal

Current type hints for enum values are incorrect because of the way Thrift compiles them in Python. E.g. for ttypes.Accuracy:

class Accuracy(object):
    TEMPORAL = 0
    SNAPSHOT = 1

So a parameter defined as def my_func(accuracy: ttypes.Accuracy = ttypes.Accuracy.TEMPORAL) ends up being an incorrect assignment because TEMPORAL is just an int.

To improve usability with IDEs and lint tools, along the same lines as #969.

Test Plan

  • Added Unit Tests
  • Covered by existing CI
  • Integration tested

Checklist

  • Documentation update

Reviewers

@earangol-stripe earangol-stripe changed the title Fix type hints for enums in ttypes Fix type hints for enums referring to ttypes Apr 15, 2025
@earangol-stripe earangol-stripe marked this pull request as ready for review April 15, 2025 22:57
@earangol-stripe earangol-stripe force-pushed the earangol/fix-enum-types branch from 79f865a to 1a26108 Compare April 16, 2025 17:04
@earangol-stripe earangol-stripe force-pushed the earangol/fix-enum-types branch from 1a26108 to 2bdd542 Compare April 16, 2025 22:14
@earangol-stripe
Copy link
Contributor Author

I just realized there is an option in the Thrift compiler to generate Python enums: py:enum. That looks better to me, though it might not be a safe change. I'll put this PR on hold and play with that option instead.

@earangol-stripe earangol-stripe marked this pull request as draft April 21, 2025 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant