Skip to content

Allow location-based parameters to create/update functions to be Optional #535

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

Closed
johnthagen opened this issue Nov 8, 2021 · 4 comments
Closed
Labels
✨ enhancement New feature or improvement

Comments

@johnthagen
Copy link
Collaborator

johnthagen commented Nov 8, 2021

Is your feature request related to a problem? Please describe.

In generated *partial_update.py modules, the type signature is:

def sync(
    id: int,
    *,
    client: AuthenticatedClient,
    form_data: T,
    multipart_data: T,
    json_body: T,
) -> Optional[T]:
    ...

This makes all of the arguments required, and means you have to call it with dummy parameters like:

        t_partial_update.sync(
            my_id,
            client=client,
            json_body=T(),  # Why are these needed if simply set to blank?
            form_data=T(color="RED"),
            multipart_data=T(),  # Why are these needed if simply set to blank?
        )

Describe the solution you'd like

It seems like if the user only wants to provide data for one of the types of request data, such as form_data they should not have to pass the others and the function would look like:

def sync(
    id: int,
    *,
    client: AuthenticatedClient,
    form_data: Optional[T] = None,
    multipart_data: Optional[T] = None,
    json_body: Optional[T] = None,
) -> Optional[T]:
    ...

And could call it like:

        t_partial_update.sync(
            my_id,
            client=client,
            form_data=T(color="RED"),
        )
@johnthagen johnthagen added the ✨ enhancement New feature or improvement label Nov 8, 2021
@dbanty
Copy link
Collaborator

dbanty commented Nov 8, 2021

I assume this is related to #453 in that there are 3 different locations for the same data, but the data itself is required. In order to have the types properly reflect the API's expected inputs, this probably needs to be a single required input which takes the location as part of it. Something like:

def sync(
    id: int,
    *,
    client: AuthenticatedClient,
    body: Body[T],
) -> Optional[T]:
    ...

where our Body class is similar to Response... something like:

from typing import Generic, TypeVar

T = TypeVar("T")
Location = TypeVar("Location")  // This will have to be set to a union of possible locations

@attr.s(auto_attribs=True)
class Body(Generic[T], Generic[Location]):
    """A response from an endpoint"""

    location: Location
    data: T

Either that or we'd have to generate multiple versions of sync and use @overload, but I'm not sure how well we could illustrate the same concept there.

Either way will get messy and we'll only want to do it in the special case where there are multiple ways to pass the same data.

Another problem is it's possible for someone to accept two payloads of the same type but from different locations at the same time. In which case, they wouldn't want the "choose one" behavior. I'm not sure how the generator could know the difference.

@johnthagen
Copy link
Collaborator Author

johnthagen commented Nov 9, 2021

this probably needs to be a single required input which takes the location as part of it.

Yes, this seems reasonable. Also thank you for linking #453, I am also using drf-spectacular so this is probably the reason it is generating all three options. As I understand it, by default DjangoRESTFramework allows multiple input types to support their developer console (presumably form_data) and normal JSON bodies for "typical" clients.


Location = TypeVar("Location")  // This will have to be set to a union of possible locations

I like the C/Rust/Javascript style comment 😆

@johnthagen johnthagen changed the title Allow parameters to partial update functions to be Optional Allow location-based parameters to create/update functions to be Optional Nov 9, 2021
@johnthagen
Copy link
Collaborator Author

drf-spectacular 0.21.0 has a new feature to support excluding certain parsers (content types) to help work around this issue:

SPECTACULAR_SETTINGS = {
    "PARSER_WHITELIST": ["rest_framework.parsers.JSONParser"],
 }

@dbanty
Copy link
Collaborator

dbanty commented Aug 13, 2023

Gonna just track this in #453 since I think they are duplicates

@dbanty dbanty closed this as not planned Won't fix, can't repro, duplicate, stale Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or improvement
Projects
None yet
Development

No branches or pull requests

2 participants