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

Incorrect type hint for content parameter of call_http in DurableOrchestrationContext.py #518

Open
louic-vermeer-pggm opened this issue Aug 6, 2024 · 3 comments
Labels
documentation About documentation good first issue Good for new contributors help wanted Good for the community input

Comments

@louic-vermeer-pggm
Copy link

louic-vermeer-pggm commented Aug 6, 2024

The content parameter in the call_http function in DurableOrchestrationContext.py has type hint Optional[str] but actually accepts any json serializable content, see lines 238-242. The type hint should be corrected.

In addition, the conditional if content and content is not isinstance(content, str) is hard to read and should probably be rewritten as if content and not isinstance(content, str).

Expected behaviour: when passing json serializable content, the type checker should not complain

@davidmrdavid
Copy link
Collaborator

Thanks @louic-vermeer-pggm. I'm open to improving the types here but first, last I checked - there was no Python type hint for "json serializable", has that changed? I realize Optional[str] is overly restrictive, but I'm curious if there's a more general (but still precise) type hint we could be using here.

@louic-vermeer-pggm
Copy link
Author

As far as I know there is no consensus on a good type hint for the json format, so I understand the choice for the restrictive Optional[str]. We may have to wait until python or its type checkers provide a solution. In the meantime I think it would at least help if the documentation was updated to indicate that json serializable content is also allowed despite the type hint.

@davidmrdavid davidmrdavid added help wanted Good for the community input good first issue Good for new contributors documentation About documentation and removed Needs: Triage 🔍 labels Sep 23, 2024
@davidmrdavid
Copy link
Collaborator

I agree with this notion. For now, I'll be labeling this as 'good first issue' and 'help wanted' in case the community would like to contribute the documentation fix. We core team has a few bug fixes to tackle before we can get to this, so contributions are welcomed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation About documentation good first issue Good for new contributors help wanted Good for the community input
Projects
None yet
Development

No branches or pull requests

2 participants