-
Notifications
You must be signed in to change notification settings - Fork 97
chore: add async_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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
700a47f to
4e3842d
Compare
977a198 to
c41a808
Compare
c41a808 to
3f26e81
Compare
vchudnov-g
left a comment
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 the extras in setup.py
What's the comment? I see that we're setting |
async_rest extra to install async rest transport dependencies
async_rest extra to install async rest transport dependenciesasync_rest extra for async rest dependencies
async_rest extra for async rest dependenciesasync_rest extra for async rest dependencies
|
|
||
|
|
||
| def default(session, install_grpc=True, prerelease=False, install_auth_aio=False): | ||
| def default(session, install_grpc=True, prerelease=False, install_async_rest=False): |
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 on pip install foo[] with an empty extras list working, which I think it does. If not, one more line will be needed.
install_extras = [] ##
if install_grpc: ##
install_extras.append("grpc")
constraints_dir = str(CURRENT_DIRECTORY / "testing")
if install_async_rest: ##
install_extras.append("async_rest")
constraints_type = "async-rest-"
else:
constraints_type = ""
if prerelease:
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 file
if not pathlib.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 that install_grpc be set if prerelease is set. But here we don't install grpc. 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 determine whether 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.
This PR adds an extra for
async_restwhich captures the minimum version ofgoogle-authwhich is required to use theasync_restfeature.