Skip to content

feat: Added support for text/yaml in OpenAPI to be used in benchling-api-client BNCH-37249 #118

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 9 commits into from
Apr 22, 2022

Conversation

miggyst
Copy link

@miggyst miggyst commented Apr 12, 2022

Added text/yaml support.
Fixed some unit tests and added 1 to check for text/yaml support

@miggyst miggyst requested a review from bowenwr April 12, 2022 19:48
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.

@miggyst miggyst requested a review from bowenwr April 20, 2022 16:25
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.

I think this looks pretty good but added @daniel-deutsch-benchling to help make sure I'm not missing anything.

I did find some issues in https://github.com/benchling/benchling-api-client/pull/21 with repeated types that may need to be addressed here.

@@ -22,6 +22,7 @@ class Response:
"application/json": "response.json()",
"application/octet-stream": "response.content",
"text/html": "response.text",
"text/yaml": "yaml.safe_load(response.text.encode('utf-8'))",
Copy link

Choose a reason for hiding this comment

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

That's a lot of logic to put in the value - is it possible to encapsulate most of that somewhere into a function that accepts response?

Suggested change
"text/yaml": "yaml.safe_load(response.text.encode('utf-8'))",
"text/yaml": "to_yaml(response)",

Copy link
Author

Choose a reason for hiding this comment

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

Broke this down on the latest commit

@miggyst miggyst requested a review from bowenwr April 22, 2022 16:33
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.

Prefer safe loader over base if it works. Otherwise, that's the last thing and I'm not going to block.

@@ -22,6 +22,7 @@ class Response:
"application/json": "response.json()",
"application/octet-stream": "response.content",
"text/html": "response.text",
"text/yaml": "yaml.safe_load(response.text.encode('utf-8'))",
Copy link

Choose a reason for hiding this comment

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

That's a lot of logic to put in the value - is it possible to encapsulate most of that somewhere into a function that accepts response?

Suggested change
"text/yaml": "yaml.safe_load(response.text.encode('utf-8'))",
"text/yaml": "to_yaml(response)",

@@ -1,6 +1,12 @@
{% macro construct(property, source, initial_value=None) %}
{% if property.required and not property.nullable %}
{% if source == "response.yaml" %}
yaml = YAML(typ="base")
Copy link

Choose a reason for hiding this comment

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

We were previously using the safe loaded from PyYaml. Does that not work here?

Suggested change
yaml = YAML(typ="base")
yaml = YAML(typ="safe")

@miggyst miggyst merged commit 97f3908 into benchling-sdk-m1-fixes-01282021 Apr 22, 2022
@miggyst miggyst deleted the bnch-37249 branch April 22, 2022 19:54
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