Skip to content
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

fix(types): remove definition of StrEnum as Enum #881

Merged
merged 9 commits into from
Jan 28, 2024

Conversation

fisher60
Copy link
Contributor

@fisher60 fisher60 commented Jan 24, 2024

This addresses an issue with typing where string enums generated by prisma are not compatible with str typing. This specifically fixes an issue where Pyright checks would fail since prisma StrEnum has type Enum, which is not valid as a str.

Change Summary

Please summarise the changes this pull request is making here.

Checklist

  • Unit tests for the changes exist
  • Tests pass without significant drop in coverage
  • Documentation reflects changes where applicable
  • Test snapshots have been updated if applicable

Agreement

By submitting this pull request, I confirm that you can use, modify, copy and redistribute this contribution, under the terms of your choice.

@RobertCraigie
Copy link
Owner

RobertCraigie commented Jan 24, 2024

Thanks, can you add type checking tests for this? https://prisma-client-py.readthedocs.io/en/stable/contributing/contributing/#type-tests

You'll have to create a new file at typesafety/pyright/types/enums.py and use the Role enum.

@RobertCraigie
Copy link
Owner

RobertCraigie commented Jan 24, 2024

I think we still need the TYPE_CHECKING block, linters unfortunately do not like try except for imports like this :(

e.g.

if TYPE_CHECKING:
  if sys.version < (3, 11):
    from strenum import StrEnum as StrEnum
  else:
    from enum import StrEnum as StrEnum
else:
  # ... original try catch

@fisher60
Copy link
Contributor Author

Thanks, can you add type checking tests for this?

I am looking into this. Are there specific tests you have in mind that are not covered by other locations that utilize enums? i.e lists.py has several locations where it utilizes enums. Do we want more involved tests than are covered here? Or some similar tests explicitly made for enums?

@fisher60 fisher60 changed the title remove definition of StrEnum as Enum fix: remove definition of StrEnum as Enum Jan 25, 2024
@fisher60
Copy link
Contributor Author

Is there a benefit to maintaining the original try-except if the if-else statement handles checking python versions and importing the StrEnum?

@RobertCraigie
Copy link
Owner

Is there a benefit to maintaining the original try-except if the if-else statement handles checking python versions and importing the StrEnum?

That's a good point, I have a personal bias towards keeping the try-except but not for any logical reasons so feel free to just use the if statement 😅

@RobertCraigie
Copy link
Owner

I am looking into this. Are there specific tests you have in mind that are not covered by other locations that utilize enums? i.e lists.py has several locations where it utilizes enums. Do we want more involved tests than are covered here? Or some similar tests explicitly made for enums?

I was thinking similar tests explicitly for enums, for example, the most important one would be a test case for the example code you shared before that broke when we were using StrEnum = Enum.

@RobertCraigie
Copy link
Owner

Looks like mypy is reporting errors from the strenum library??

@RobertCraigie
Copy link
Owner

The pyright lint failure in CI is likely because we're using 3.11 to run & install dependencies but targeting 3.7, IIRC we can't use 3.7 to install dependencies because some dev dependencies have dropped support for 3.7 and I didn't want to drop 3.7 support until there was a user-facing need to do so.

I think this'll be a problem for me as well locally as I use 3.11 when developing, so I think the solution would be to add strenum to our internal dev dependencies at pipelines/requirements/dev.txt.

@RobertCraigie
Copy link
Owner

@fisher60 the lint error is because you need to import StrEnum as StrEnum 🙃

@RobertCraigie RobertCraigie changed the title fix: remove definition of StrEnum as Enum fix(types): remove definition of StrEnum as Enum Jan 26, 2024
@RobertCraigie
Copy link
Owner

RobertCraigie commented Jan 26, 2024

hmm looks like mypy really doesn't like our usage of strenum... maybe we should just revert to the previous enum definition at the type-level as the only issues that had were at runtime, e.g.

if TYPE_CHECKING:
  if sys.version_info >= (3, 11):
    from enum import StrEnum as StrEnum
  else:
    class StrEnum(str, Enum):
      ...
else:
  if sys.version_info >= (3, 11):
    from enum import StrEnum as StrEnum
  else:
    from strenum import StrEnum as StrEnum

This addresses an issue with typing where string enums generated by prisma are not compatible with str typing. This specifically fixes an issue where Pyright checks would fail since prisma StrEnum has type Enum, which is not valid as a str.
As per the discussion in the related pull request, I have added back the TYPE_CHECKING check to prevent issues with linters for the try-except import. I have left the try-except in as-per the discussion, but I am unsure of the benefit to having both the check and the try-except.
We are now explicitly checking the python version being used when deciding which StrEnum to import, so the try-except is no longer needed. Since the try-except was removed, the `if TYPE_CHECKING` check does not do much here, so I removed it as well.
…t schema.prisma, add dev dependency for strenum package
This was erroneously removed and should now be correct again.
attempting to import strenum was causing mypy errors, so I have refactored the StrEnum to either be typed as StrEnum from enum, or as (str, Enum). This seems to be compatible with both pyright and mypy for typing.
@fisher60 fisher60 force-pushed the fix/string-enum-typing branch from ece35bf to 212f179 Compare January 27, 2024 05:12
this should no longer be necessary due to previous changes
Copy link
Owner

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

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

Thank you!!

@RobertCraigie RobertCraigie merged commit 6485828 into RobertCraigie:main Jan 28, 2024
30 checks passed
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.

2 participants