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

added backwards compat support and resolved issue with 3.11 #340

Closed
wants to merge 1 commit into from

Conversation

slapglif
Copy link

In my pull request, I made changes to the main function in order to address an error. I modified the function signature by changing the type hint for the out_path parameter. Originally, it was annotated as str | None, which uses the union operator. However, this syntax is not compatible with older versions of Python. To make it compatible, I replaced it with Union[str, None] using the typing.Union class. This change ensures that the code can be executed in Python versions prior to 3.10. You can review the specific modifications I made, should not effect anything downstream.

@slapglif
Copy link
Author

slapglif commented Jun 22, 2023

on why this is relevant:

fastapi/typer#371
fastapi/typer#533

I digress, I booted it up and it failed so I shared my fix :)

@@ -12,7 +13,7 @@
@app.command()
def main(
messages_path: str,
out_path: str | None = None,
out_path: Union[str, None] = None,
Copy link
Author

Choose a reason for hiding this comment

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

I suppose I would point out, isnt this a bit redundant? Optional[str] by its self works fine but if you need the field set then theres two best practices here:

  1. Optional[str] = None # gives us a default value of None for the field, is polymorphic.
  2. Optional[str] = Field(...) # gives us a default of None with an enforced str type validation - all classes should be dataclasses anyway right?

@patillacode
Copy link
Collaborator

Thanks @slapglif

I just merged this from another PR, thanks anyway!

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.

3 participants