Conversation
|
|
||
| - 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.
Missing Oxford comma here, but this is probably the mother of all nits. :D
noxfile.py
Outdated
| session.install("mock", "pytest") | ||
|
|
||
| session.install("-e", "test_utils") | ||
| session.install("-e", "psutil") |
There was a problem hiding this comment.
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.
@hongalex I think you were looking at the psutil issue?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Thanks for the quick fix of the synthtool!
There was a problem hiding this comment.
I pushed a commit with the regeneration. PTAL and merge when you're ready. :)
| 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.
Nice, thanks! ❤️
BTW, is now time to remove the local test_utils directory? (I can do it, in a separate PR)
|
|
||
| session.install("-e", ".") | ||
| session.install("sphinx", "alabaster", "recommonmark") | ||
| session.install("sphinx<3.0.0", "alabaster", "recommonmark") |
There was a problem hiding this comment.
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