Skip to content
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

Problem: leading _ in name is removed by PythonClientCodegen #3548

Closed
wants to merge 1 commit into from

Conversation

dkliban
Copy link
Contributor

@dkliban dkliban commented Aug 3, 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

Solution: move leading _ to the end of the name

Double leading underscores in Python attribute names cause them to have their names
mangled as described in PEP 8[0]. That's why they were being removed completely before
this patch.

Due to the implementation details of the python models that are generated, leaving a
single leading underscore causes a conflict between setters of variables name that differ
only by a leading underscore. e.g.: 'name' and '_name'.

This patch changes how leading underscores in variables and parameters are handled. All
leading underscores are moved to the end of the name.

[0] https://www.python.org/dev/peps/pep-0008/

@dkliban dkliban force-pushed the python-respect-strict branch 2 times, most recently from d65a3aa to b8e93b4 Compare August 3, 2019 15:18
Solution: move leading _ to the end of the name

Double leading underscores in Python attribute names cause them to have their names
mangled as described in PEP 8[0]. That's why they were being removed completely before
this patch.

Due to the implementation details of the python models that are generated, leaving a
single leading underscore causes a conflict between setters of variables name that differ
only by a leading underscore. e.g.: 'name' and '_name'.

This patch changes how leading underscores in variables and parameters are handled. All
leading underscores are moved to the end of the name.

[0] https://www.python.org/dev/peps/pep-0008/
@dkliban dkliban changed the title Problem: _ removed by PythonClientCodegen when --strict-spec=false Problem: leading _ in name is removed by PythonClientCodegen Aug 3, 2019
@jimschubert
Copy link
Member

cc python technical community for review:
@taxpon @frol @mbohlool @cbornet @kenjones-cisco @tomplus @Jyhess

The target spec for this work defined type as well as _type, and this caused issues. I provided some feedback in Slack, but I've only used Python minimally. The approach in this PR looks good to me, but does anyone else have any concerns with merging it?

@dkliban
Copy link
Contributor Author

dkliban commented Aug 6, 2019

This patch resolves some of the concerns raised here #3168

@wing328
Copy link
Member

wing328 commented Aug 6, 2019

Looks like this is a breaking change without fallback?

@dkliban
Copy link
Contributor Author

dkliban commented Aug 6, 2019

Looks like this is a breaking change without fallback?

Yes, this is a breaking change. However, I don't think the existing behavior was correct.

How can we make this change less painful for existing users?

@dkliban
Copy link
Contributor Author

dkliban commented Nov 22, 2019

I decided to change the parameter names in my OpenAPI schema and no longer have this problem. I am closing this PR.

@dkliban dkliban closed this Nov 22, 2019
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.

None yet

4 participants