Skip to content

feat: Change optional Open API query parameters to allow None BNCH-20153 #27

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
wants to merge 7 commits into from

Conversation

forest-benchling
Copy link

@forest-benchling forest-benchling commented Jan 11, 2021

Fixes openapi-generators#285.

Instead of methods like

def sync_detailed(
    *,
    client: Client,
    size: Union[Unset, int] = UNSET,
) -> Response[Thing]:

we will now generate

def sync_detailed(
    *,
    client: Client,
    size: Union[Unset, None, int] = None,
) -> Response[Thing]:

and treat None the same as Unset.

This is a bit different from the original proposal in the issue, but if you scroll down you can see that it's what was agreed upon in the end IIUC (though I wasn't sure if we agreed on what the default value should be). In addition, it has the benefit of preserving back-compat.

I will submit a PR upstream, but first wanted to get early feedback internally.

@forest-benchling forest-benchling marked this pull request as ready for review January 12, 2021 22:49
@forest-benchling forest-benchling changed the title feat: Change optional Open API query parameters to use Optional[str] instead of Unset BNCH-20153 feat: Change optional Open API query parameters to allow None BNCH-20153 Jan 12, 2021
Copy link

@bowenwr bowenwr left a comment

Choose a reason for hiding this comment

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

Awesome work, I pulled this in to the client and ran the SDK tests against it. Fixed the functional issues and appeased MyPy. Let's get this open upstream! Thank you!

Copy link

@packyg packyg left a comment

Choose a reason for hiding this comment

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

LGTM. Can't say I love the proliferation of args to functions and templates, but doesn't seem to be a way around it.

Approving but agree upstreaming would be the ideal way to incorporate this


def get_type_string(self, no_optional: bool = False) -> str:
def get_type_string(self, no_optional: bool = False, query_parameter: bool = False) -> str:
Copy link

Choose a reason for hiding this comment

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

Would it make more sense to label this flag more to what it does than how it's used along the lines of

Suggested change
def get_type_string(self, no_optional: bool = False, query_parameter: bool = False) -> str:
def get_type_string(self, no_optional: bool = False, treat_none_as_unset: bool = False) -> str:

(I don't think I really like treat_none_as_unset, but you get my point)

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, interesting idea. I could see that being useful in a future world where treat_none_as_unset might be used in different situations. Since for now we only have one use case, which is query parameters, I think it's more descriptive to keep it as is (especially if we end up changing the behavior of query parameters again).

@eli-bl eli-bl deleted the forest-unset branch November 22, 2024 19:42
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.

Use Optional for Query Params Instead of Union[Unset, ...]
3 participants