Skip to content

feat: Add support for cookie parameters #326

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 16, 2021
Merged

feat: Add support for cookie parameters #326

merged 4 commits into from
Feb 16, 2021

Conversation

dongfangtianyu
Copy link
Contributor

close #325

@dongfangtianyu dongfangtianyu force-pushed the dev branch 2 times, most recently from 95ae020 to 9ce9760 Compare February 5, 2021 10:49
@codecov
Copy link

codecov bot commented Feb 5, 2021 •
edited
Loading

Codecov Report

Merging #326 (503ab87) into main (d535bd0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #326   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1394      1398    +4     
=========================================
+ Hits          1394      1398    +4
Impacted Files Coverage Δ
openapi_python_client/parser/openapi.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d535bd0...d468185. Read the comment docs.

emann
emann requested changes Feb 5, 2021
View reviewed changes
Copy link
Collaborator

@emann emann left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround on this @dongfangtianyu, looking good so far! If you could add some cookie parameters to an endpoint and/or make a new endpoint that uses cookie parameters in end_to_end_tests/openapi.json and run task re that'd be great - it gives us a chance to see the code actually get generated and check that everything is kosher.

@dongfangtianyu
Copy link
Contributor Author

Thinks , @emann !
This made me learn the new usage of poetry, and I'll do it right away

dbanty
dbanty requested changes Feb 8, 2021
View reviewed changes
Copy link
Collaborator

@dbanty dbanty left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, just one question / suggestion around transforming cookie names. I believe that we don't want to do this the same way we were for headers. I don't really use cookies though so I could be wrong.

Actually, @sweetb probably knows best here.

openapi_python_client/templates/endpoint_macros.py.jinja Outdated
Comment on lines 18 to 21
cookies["{{ parameter.python_name | kebabcase}}"] = {{ parameter.python_name }}
{% else %}
if {{ parameter.python_name }} is not UNSET:
cookies["{{ parameter.python_name | kebabcase}}"] = {{ parameter.python_name }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that cookies are both case-sensitive and don't follow the same kebab-case convention as headers. So we probably want this to be:

Suggested change
cookies["{{ parameter.python_name | kebabcase}}"] = {{ parameter.python_name }}
{% else %}
if {{ parameter.python_name }} is not UNSET:
cookies["{{ parameter.python_name | kebabcase}}"] = {{ parameter.python_name }}
cookies["{{ parameter.name}}"] = {{ parameter.python_name }}
{% else %}
if {{ parameter.python_name }} is not UNSET:
cookies["{{ parameter.name}}"] = {{ parameter.python_name }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right.
I will update the test case and commit it again

@dbanty dbanty requested a review from sweetb February 8, 2021 18:15
sweetb
sweetb reviewed Feb 9, 2021
View reviewed changes
openapi_python_client/templates/endpoint_macros.py.jinja Outdated
cookies["{{ parameter.name}}"] = {{ parameter.python_name }}
{% else %}
if {{ parameter.python_name }} is not UNSET:
cookies["{{ parameter.python_name | kebabcase}}"] = {{ parameter.python_name }}
Copy link

Choose a reason for hiding this comment

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

Suggested change
cookies["{{ parameter.python_name | kebabcase}}"] = {{ parameter.python_name }}
cookies["{{ parameter.name }}"] = {{ parameter.python_name }}

Don't you still want parameter.name set here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed that

sweetb
sweetb previously approved these changes Feb 15, 2021
View reviewed changes
Copy link

@sweetb sweetb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Approved

emann
emann approved these changes Feb 15, 2021
View reviewed changes
Copy link
Collaborator

@emann emann left a comment

Choose a reason for hiding this comment

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

LGTM as well - thanks again @dongfangtianyu!

emann
emann requested changes Feb 15, 2021
View reviewed changes
Copy link
Collaborator

@emann emann left a comment

Choose a reason for hiding this comment

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

Almost forgot - please add an entry to the changelog and give yourself credit :)

dongfangtianyu added a commit to dongfangtianyu/openapi-python-client that referenced this pull request Feb 16, 2021
@dongfangtianyu
docs: Add CHANGELOG entry for openapi-generators#326
7454421 
# Conflicts:
#	CHANGELOG.md
@dbanty dbanty requested review from emann and dbanty February 16, 2021 19:57
@dbanty dbanty merged commit 2d305dd into openapi-generators:main Feb 16, 2021
@dbanty
Copy link
Collaborator

dbanty commented Feb 16, 2021

Great, thanks everyone!

@dbanty dbanty added this to the 0.8.0 milestone Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sweetb sweetb sweetb left review comments

@emann emann Awaiting requested review from emann

@dbanty dbanty Awaiting requested review from dbanty

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0
Development

Successfully merging this pull request may close these issues.

Add support for header and cookie parameters
4 participants
@dongfangtianyu @dbanty @sweetb @emann