Skip to content

fix: Generate parameters to generated client without adding None BNCH-22940 #77

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

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

GitOnUp
Copy link

@GitOnUp GitOnUp commented Jul 8, 2021

Upstream generates the client this way, and in a parallel benchling-sdk ticket we'll be doing a conversion to UNSET for None parameters in the service layer. Making this change in the fork in the meantime will keep us in a good state.

@GitOnUp GitOnUp requested a review from forest-benchling July 8, 2021 17:42
@GitOnUp
Copy link
Author

GitOnUp commented Jul 8, 2021

William was reviewing this set of changes on the benchling-sdk side for https://github.com/benchling/benchling-sdk/pull/149, but has indicated he's about to be OOO.

Copy link

@forest-benchling forest-benchling left a comment

Choose a reason for hiding this comment

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

Is this just so that our switching onto the upstream is more incremental?

@GitOnUp
Copy link
Author

GitOnUp commented Jul 8, 2021

@forest-benchling yes, and so that we don't continue to accidentally try to call the client functions with None in the interim period before the switch.

@forest-benchling
Copy link

@forest-benchling yes, and so that we don't continue to accidentally try to call the client functions with None in the interim period before the switch.

@GitOnUp 👍 . Just to clarify, that this is only a matter of type hinting--we're still allowing runtime calls with None, IIUC.

@GitOnUp GitOnUp merged commit 0e1afa8 into benchling-sdk-m1-fixes-01282021 Jul 8, 2021
@eli-bl eli-bl deleted the george/unset-as-none branch November 26, 2024 22:45
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.

2 participants