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

[Python][Client] pure library client package #470

Merged
merged 8 commits into from
Jul 16, 2018

Conversation

rienafairefr
Copy link
Contributor

@rienafairefr rienafairefr commented Jul 5, 2018

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 and ./bin/security/{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\.
    -> Caveat I wasn't sure if I needed to commit the changes that happened after running /bin/python-petstore.sh, I commited them just in case
  • Filed the PR against the correct branch: master, 3.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
    @taxpon @frol @mbohlool @cbornet @kenjones-cisco

Description of the PR

A few times already, when using Swagger-codegen before also, I was thinking that all the supporting files for Python packages are a lot of the time unecessary. If you're building a package around auto-generated code, you'll be using it as a library and have your own setup.py, etc.

This PR adds a "onlyPackage" cli options ([...]generate [...] -l python -DonlyPackage [...]) (couldn't think of a better name, possibilities "libraryClient", "pureLibrary", ... IDK)

When this option is activated,

  • docs and tests are generated in subfolders of the package itself
  • README.mustache is generated to <packageName>_README.md to not overwrite the overarching project's README.md
  • all the supporting file at the root of the project (git_push, requirements, setup.py) are not generated

@@ -198,10 +200,21 @@ public void processOpts() {
setPackageVersion("1.0.0");
}

if (additionalProperties.containsKey(CodegenConstants.ONLYPACKAGE_GENERATION)) {
setOnlyPackage(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is a new feature, and is not overriding an inherited functionality. The pattern used for excludeTests would seem to be a better match. ie using a local method variable instead of having a class variable with getter/setter.

@kenjones-cisco
Copy link
Contributor

Thanks for the PR!
Overall this looks good. Just a minor change is recommended.

@kenjones-cisco
Copy link
Contributor

LGTM!

@rienafairefr
Copy link
Contributor Author

@kenjones-cisco you're right, a local variable makes more sense. I've pushed that change up 👍

@rienafairefr
Copy link
Contributor Author

the fail in the CI is normal I think, something related to the spring-mvc-j8-localdatetime sample

@cbornet
Copy link
Member

cbornet commented Jul 6, 2018

Is it not already possible to generate this with the existing selector options ?

@wing328
Copy link
Member

wing328 commented Jul 6, 2018

If you're building a package around auto-generated code, you'll be using it as a library and have your own setup.py, etc

What about using ".openapi-generator-ignore" to skip those files?

@rienafairefr
Copy link
Contributor Author

I usually don't want to fully skip them, just generate them somewhere else. For example, if there is already a docs/ folder in my project, and I auto-generate there, openapi-generator would interfere with it. docs/ inside the package folder is good, same for tests, same for the readme.md. I've tested a lot of setups (ignoring files, copying the files after generation), and I feel like the simplest is having a flag for the generator. All in all, I feel like generating a library instead of a Pypi package should be an option :-)

@wing328
Copy link
Member

wing328 commented Jul 13, 2018

@rienafairefr as discussed, please name the option as "generateSourceCodeOnly" (default: false).

@rienafairefr
Copy link
Contributor Author

OK, I just renamed the option to generateSourceCodeOnly 👍

@rienafairefr
Copy link
Contributor Author

I rebased on latest master to remove the merge conflict due to version number change (master at 3.1.1 instead of 3.1.0 if I understood correctly)

@wing328 wing328 merged commit 84ef98f into OpenAPITools:master Jul 16, 2018
@rienafairefr rienafairefr deleted the python-client-library branch July 16, 2018 07:53
@wing328
Copy link
Member

wing328 commented Jul 16, 2018

@rienafairefr thanks for the PR, which has been merged into the master.

@rienafairefr
Copy link
Contributor Author

@wing328 You're welcome 👍

@fizxmike
Copy link

fizxmike commented Jul 20, 2018

FWIW, having the library code in the same repo as your implementation code seems to be a bad pattern (maybe okay for POC). Why not just put the python client in it's own repo then do pip install git+https://github.com/somebodys/pythonclient ??

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Python client pure library package

* check onlyPackage CLI option

* run /bin/python-petstore.sh, update the python samples for CI

* onlyPackage local variable instead of classp property

* fix CI: __future__ absolute_import must be first in file

* update samples

* generateSourceCodeOnly

* updated samples
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

5 participants