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

Inconsistent typing used in generated code and the libraries. #333

Closed
andrueastman opened this issue Sep 13, 2024 · 2 comments
Closed

Inconsistent typing used in generated code and the libraries. #333

andrueastman opened this issue Sep 13, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@andrueastman
Copy link
Member

andrueastman commented Sep 13, 2024

Related to #331

Running typechecking validation using generated SDK by Kiota yields errors due to the inconsistencies .

1. Usage of the ParsableFactory

The functions below use the type as a parameter

However, on generation, the parameters passed are not instances of the factory but type(class) definitions.

The relevant implementations also happen to call the create_from_discriminator_value statically directly.
https://github.com/microsoft/kiota-serialization-json-python/blob/160d8a6d54237f96205f9972acbf17da5d80b2cd/kiota_serialization_json/json_parse_node.py#L215C18-L215C25

Resolving the typing inconsistency will need both generation and library changes as if we update the ParsableFactory to be a type variable as it is used and rename the current ParsableFactory we may end up breaking library functionalities calling methods like create_from_discriminator_value directly.

We probably need to either

  • create a new type variable based of the ParsableFactory class (or generate it inline) and generate it across the SDK to fix the type inconsistency and update the library parameter types to use this too.
  • update the library implementation of ParsableFactory to be consistent with other sdks and have the ParsableFactory be a function template. This would also involve updating the library implementations like get_object_value to no longer expect types and also probably be source breaking as well.

2. Return type of request executors should be optional in the case of returning streams/bytes
Taking a look at an example at
https://github.com/microsoftgraph/msgraph-sdk-python/blob/416fccc20bd90a8f8d71ef7cc7519d2f63f488f5/msgraph/generated/applications/item/logo/logo_request_builder.py#L49

The function ends up returning the result of send_primitive_async which is an optional not just bytes

@andrueastman andrueastman added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Sep 13, 2024
@andrueastman andrueastman changed the title ParsableFactory type is inconsistently used in generated code and the libraries. Inconsistent typing used in generated code and the libraries. Sep 13, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Sep 23, 2024
@andrueastman andrueastman moved this from Needs Triage 🔍 to Todo 📃 in Kiota Sep 23, 2024
@andrueastman
Copy link
Member Author

Also related to microsoftgraph/msgraph-sdk-python#851

@andrueastman
Copy link
Member Author

Resolved with #331

@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✔️ in Kiota Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

1 participant