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

Add support for Python #26 #27

Closed

Conversation

fullcircle23
Copy link
Contributor

@fullcircle23 fullcircle23 commented Feb 27, 2020

Added support for Python openapi generation only. The publish and release of the openapi python package to PyPi will be done as a separate task #28

@fboucquez fboucquez self-requested a review February 27, 2020 12:21
generate.sh Outdated
--additional-properties="packageVersion=$VERSION" \
--additional-properties="snapshot=$SNAPSHOT" \
--type-mappings=x-number-string=int
echo "DONE"
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove echo "Done"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ;-)

--additional-properties="projectName=$ARTIFACT_ID" \
--additional-properties="packageVersion=$VERSION" \
--additional-properties="snapshot=$SNAPSHOT" \
--type-mappings=x-number-string=int
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not expert in python, is int good enough for unsigned long / BigInteger without overflowing?

  • Double-check if the client can parse unsigned string numbers like "1683298087010368300" from rest into 1683298087010368300 int number
  • Double-check if the client can serialize int number like 1683298087010368300 into string number "1683298087010368300" when sending a payload.

The x-number-string is mapper customization so the user doesn't need to convert from string to number every time there is a string number. Check if it works when creating int from string numbers.

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, in fact int is the only kind of integer since Python 3.0. Python 2.x had ints and long ints which were unified in PEP-237 and fully implemented in Python 3.x.

Parsing and serialising of very large int numbers were tested as part of catbuffer-generators for Python.

Yes x-number-string mapper customisation works when creating int from string numbers.

generate.sh Outdated
-t "$LIBRARY-templates/" \
-i "$INPUT" \
--additional-properties="projectName=$ARTIFACT_ID" \
--additional-properties="packageVersion=$VERSION" \
Copy link
Contributor

Choose a reason for hiding this comment

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

packageName is openapi_clientby default.

https://openapi-generator.tech/docs/generators/python

Maybe we could change it to symbol_openapi_client so it look better when sdk or libs are importing the classes?

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. Passed additional property packageName symbol_openapi_client.

--additional-properties="snapshot=$SNAPSHOT" \
--type-mappings=x-number-string=int
echo "DONE"
return 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to at least compile the generated code to validate if it's fine. Are you planning to target the lib to multiple python versions? Maybe requiring python 3.7+ is too restrictive?

There is a generated .travis file in the build/symbol-openapi-python-client folder. It can give you a hint about how to compile it.

Copy link
Contributor Author

@fullcircle23 fullcircle23 Mar 2, 2020

Choose a reason for hiding this comment

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

Python code is validated by running tests and/or using linters. Linters are great for your own team's code but not for code generated by an external project as the rules will be different.

The openapi-generator generates a test suite to validate the generated code and all tests pass. Automation of running the test suite in travisci will be done separately, probably with #28

I've lowered the requirement to python 3.4+. However, the recommended is 3.7+ for compatibility with Python Catbuffer library. Also note that Python 3.4 has already reached End Of Life.

I've included a .travis file but as mentioned earlier will be working on the release/publish as a separate task #28

@fullcircle23 fullcircle23 requested a review from fboucquez March 2, 2020 12:02
@fullcircle23 fullcircle23 changed the base branch from master to python-support April 7, 2020 06:45
@fullcircle23
Copy link
Contributor Author

Closing this PR. Will be opening a new PR after fixing python dependencies on Trusty build.

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