-
Notifications
You must be signed in to change notification settings - Fork 22
Use mypy annotations #9
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,30 +1,36 @@ | ||
| # Copyright 2018 Easymov Robotics | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| from typing import Optional | ||
|
|
||
| import os | ||
| import shutil | ||
|
|
||
| from colcon_core.environment_variable import EnvironmentVariable | ||
|
|
||
| """Environment variable to override the Cargo executable""" | ||
| # Environment variable to override the Cargo executable | ||
| CARGO_COMMAND_ENVIRONMENT_VARIABLE = EnvironmentVariable( | ||
| 'CARGO_COMMAND', 'The full path to the Cargo executable') | ||
|
|
||
|
|
||
| def which_executable(environment_variable, executable_name): | ||
| def which_executable(environment_variable: str, executable_name: str) \ | ||
| -> Optional[str]: | ||
| """ | ||
| Determine the path of an executable. | ||
|
|
||
| An environment variable can be used to override the location instead of | ||
| relying on searching the PATH. | ||
| :param str environment_variable: The name of the environment variable | ||
| :param str executable_name: The name of the executable | ||
| :rtype: str | ||
|
|
||
| :param environment_variable: The name of the environment variable | ||
| :param executable_name: The name of the executable, or None if that's not | ||
| found | ||
| """ | ||
| value = os.getenv(environment_variable) | ||
| if value: | ||
| return value | ||
| return shutil.which(executable_name) | ||
| if environment_variable in os.environ: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is equivalent to the existing code, why the change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find queyring directly the
In any case, it's not terribly important, I can revert the change if you think it was better before. |
||
| return os.environ[environment_variable] | ||
|
|
||
| which = shutil.which(executable_name) | ||
| if which: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This already returns return shutil.which(executable_name)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, agree, I 'll change it back |
||
| return which | ||
|
|
||
|
|
||
| CARGO_EXECUTABLE = which_executable( | ||
|
|
||
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'd keep this as docstring
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.
Depends what you use to generate the documentation, but if we're talking about sphinx, it doesn't support extracting docstrings from variables unfortunately