-
Notifications
You must be signed in to change notification settings - Fork 207
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
[CHANGE ME] Re-generated to pick up changes in the API or client library generator. #66
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.
More of a quick review, but the test dependency installation is messed up in noxfile. We either need to fix the template, or make a manual adjustment in synth.py
.
@@ -22,7 +22,7 @@ In order to add a feature: | |||
documentation. | |||
|
|||
- The feature must work fully on the following CPython versions: 2.7, | |||
3.5, 3.6, and 3.7 on both UNIX and Windows. | |||
3.5, 3.6, 3.7 and 3.8 on both UNIX and Windows. |
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.
Missing Oxford comma here, but this is probably the mother of all nits. :D
noxfile.py
Outdated
|
||
session.install("-e", "test_utils") | ||
session.install("-e", "psutil") |
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.
Psutil is an external test dependency, why has it been changed to an editable install? This looks like something that could fail.
Edit: Indeed, that's the cause of the failing system tests.
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.
@hongalex I think you were looking at the psutil issue?
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.
Hm, yeah I added psutil in this PR but I didn't realize it was an editable install. Every time we try to merge in a gapic generated PR, it tries to remove psutil from the dependencies, so we have to manually add it in.
@busunkim96, I tried looking for other places where psutil should be (re)added, but can't seem to find it. Any ideas?
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 asked Alex to add the extra dependency via synth.py so it doesn't get overridden in subsequent PRs. But the noxfile template incorrectly assumes that all extra dependencies are local. :) I'll open a PR in synthtool.
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.
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.
Thanks for the quick fix of the synthtool!
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 pushed a commit with the regeneration. PTAL and merge when you're ready. :)
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.
Looks good IMO.
session.install("-e", "test_utils") | ||
session.install("-e", "psutil") | ||
session.install("mock", "pytest", "psutil") | ||
session.install("git+https://github.com/googleapis/python-test-utils") |
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.
Nice, thanks! ❤️
BTW, is now time to remove the local test_utils
directory? (I can do it, in a separate PR)
@@ -141,7 +139,7 @@ def docs(session): | |||
"""Build the docs for this library.""" | |||
|
|||
session.install("-e", ".") | |||
session.install("sphinx", "alabaster", "recommonmark") | |||
session.install("sphinx<3.0.0", "alabaster", "recommonmark") |
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.
Pinning Sphinx to < 3.x
is not needed anymore, we fixed all the warnings in #70.
Not a show-stopper, though, just something we can clean in the next autogen pass.
Edit:
It turns out that some of the new changes introduced new Sphinx warnings, thus the pin is (again) necessary until it gets fixed.
docstring of google.cloud.pubsub_v1.types.AcknowledgeRequest:3:Unexpected indentation.
@busunkim96 Is this generator issue known, i.e. accidentally introducing docs warnings in Sphinx 3.x? Do we have a feedback loop mechanism to make this visible to generator maintainers?
This PR was generated using Autosynth. 🌈
Log from Synthtool