Skip to content

Conversation

@wing328
Copy link
Member

@wing328 wing328 commented Aug 13, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

To fix #3485

Tested locally with url, http, ioutil and context as required parameters.

cc @antihax (2017/11) @bvwells (2017/12) @grokify (2018/07) @kemokemo (2018/09) @bkabrda (2019/07)

@wing328 wing328 added this to the 4.1.1 milestone Aug 13, 2019
@bkabrda
Copy link
Contributor

bkabrda commented Aug 13, 2019

IIUC this tries to address the problem by using more "obscured" version of the words - so technically if someone used a query parameter _context, this would still fail. I guess there's a low enough probability of getting in that situation that I'm 👍 (as I don't see any better solution other than obscuring the import names even more, e.g. _openapiGeneratorImportContext, but I guess that's too far fetched).

So overall, 👍 from me.

@wing328
Copy link
Member Author

wing328 commented Aug 13, 2019

so technically if someone used a query parameter _context, this would still fail.

Very good point. My understanding is that the query (and other forms of) parameter names are sanitized to conform to Go variable naming convention so the leading understood will be removed.

I did a test locally with _context and confirmed no variable naming conflict but I'm happy to take another look if you can repeat the issue.

@bkabrda
Copy link
Contributor

bkabrda commented Aug 13, 2019

I can't reproduce such an issue, so you're right, it's ok like this 👍

Could you please add the same modification to the go-experimental client to keep them in sync as much as possible? Thanks!

@wing328
Copy link
Member Author

wing328 commented Aug 13, 2019

Could you please add the same modification to the go-experimental client to keep them in sync as much as possible? Thanks!

Sure I can do it later this week.

@wing328
Copy link
Member Author

wing328 commented Aug 13, 2019

I will also add more test cases in a separate PR.

@wing328 wing328 merged commit 5578611 into master Aug 14, 2019
@wing328 wing328 deleted the go_url branch August 14, 2019 07:17
@wing328
Copy link
Member Author

wing328 commented Aug 15, 2019

FYI. Added more test cases via #3640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][Golang][Client] reserved word collision for required API parameter

3 participants