- Notifications
You must be signed in to change notification settings - Fork90
chore: addasync_rest extra for async rest dependencies#701
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
700a47f to4e3842dCompare977a198 toc41a808Comparec41a808 to3f26e81CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
One non-trivial comment about using theextras insetup.py
What's the comment? I see that we're setting |
async_rest extra to install async rest transport dependenciesasync_rest extra to install async rest transport dependenciesasync_rest extra for async rest dependenciesasync_rest extra for async rest dependenciesasync_rest extra for async rest dependenciesUh oh!
There was an error while loading.Please reload this page.
| @@ -110,7 +111,7 @@ def install_prerelease_dependencies(session, constraints_path): | |||
| session.install(*other_deps) | |||
| def default(session, install_grpc=True, prerelease=False,install_auth_aio=False): | |||
| def default(session, install_grpc=True, prerelease=False,install_async_rest=False): | |||
vchudnov-gSep 30, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 suggest restructuring just slightly for brevity (I marked with## the sections I changed). This depends onpip install foo[] with an empty extras list working, which I think it does. If not, one more line will be needed.
install_extras= []##ifinstall_grpc:##install_extras.append("grpc")constraints_dir=str(CURRENT_DIRECTORY/"testing")ifinstall_async_rest:##install_extras.append("async_rest")constraints_type="async-rest-"else:constraints_type=""ifprerelease:install_prerelease_dependencies(session,f"{constraints_dir}/constraints-{constraints_type}{PYTHON_VERSIONS[0]}.txt", )# This *must* be the last install command to get the package from source.session.install("-e",f".[{','.join(install_extras)}]","--no-deps"## see my other comment about installing grpc )else:constraints_file= (f"{constraints_dir}/constraints-{constraints_type}-{session.python}.txt" )# fall back to standard constraints fileifnotpathlib.Path(constraints_file).exists():constraints_file=f"{constraints_dir}/constraints-{session.python}.txt"session.install("-e",f".[{','.join(install_extras)}]",##"-c",constraints_file, )
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.
addressed.
noxfile.py Outdated
| ".[grpc]" if install_grpc else ".", | ||
| "-c", | ||
| f"{constraints_dir}/constraints-{session.python}.txt", | ||
| "-e", "." + ("[async_rest]" if install_async_rest else ""), "--no-deps" |
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.
This was a pre-existing issue: at the top of the function we require thatinstall_grpc be set ifprerelease is set. But here we don't installgrpc. Is that an oversight? It seems to me we probably should install it here.
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.
addressed.
| ) | ||
| # fall back to standard constraints file | ||
| if not pathlib.Path(constraints_file).exists(): | ||
| constraints_file = f"{constraints_dir}/constraints-{session.python}.txt" |
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.
Add a reminder at the top of the else that the constraints file does not determinewhether the listed imports are imported.
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.
addressed.
84bf637 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR adds an extra for
async_restwhich captures the minimum version ofgoogle-authwhich is required to use theasync_restfeature.