-
Notifications
You must be signed in to change notification settings - Fork 98
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
Accelerate YAML parsing when LibYAML is available #377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me, could you fix the pre-commit and mypy issue? I'll be happy to merge it in once the build passes.
Thank you for your reply. I updated this pull request to pass all the tests. For the mypy issue, I simply skipped the line in the same manner as people discussing in the mypy repository: python/mypy#1153 |
tox.ini
Outdated
@@ -11,6 +11,12 @@ deps = | |||
commands = | |||
python -m pytest --cov --capture=no --benchmark-skip {posargs:tests} | |||
|
|||
[testenv:py37] | |||
commands = | |||
# cover the case that native libyaml is not avaiable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should modify the standard py37
test environment for that case. Either we create a separate one or we just don't test this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to just remove your changes to this file, and I will merge your branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had trouble keeping the coverage check and figured out that we should modify test environments to cover the case that native libyaml is not available.
Thus, as you suggested, I will separate the case as a new environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry too much about the coverage check, it's ok for it to go down a bit with changes like these. Looking at the latest changes, I feel you're trying to do too much to test what essentially is a documented feature of PyYAML. I really think we can go ahead with just the changes to bravado_core/spec.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you won't mind the degradation of the coverage rate, I removed the newly added test condition. I think it now ready to be merged.
After hours of investigation, I found there are several issues in pip and pyyaml which we cannot easily address:
Thus, I decided to explicitly skip building pyyaml in the case of Python 2.7. I believe this is a reasonable way under these restrictions. |
Thanks for your contribution @hiromu! |
When a Swagger definition is provided in YAML, the current implementation uses a Python loader for parsing.
However, it can be accelerated by using a native loader when LibYAML is available.