-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Enable warn_unreachable
mypy option
#17365
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.
Great stuff!
lib/galaxy/tool_util/toolbox/base.py
Outdated
@@ -758,7 +758,7 @@ def get_tool(self, tool_id, tool_version=None, get_all_versions=False, exact=Fal | |||
def has_tool(self, tool_id: str, tool_version: Optional[str] = None, exact: bool = False): | |||
return self.get_tool(tool_id, tool_version=tool_version, exact=exact) is not None | |||
|
|||
def is_missing_shed_tool(self, tool_id: str) -> bool: | |||
def is_missing_shed_tool(self, tool_id: Optional[str]) -> bool: |
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.
In the only place from which this is called ( lib/galaxy/webapps/galaxy/services/workflows.py
), the tool_id
passed is never None, so maybe keep the previous annotation and drop lines 763-765?
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.
step.tool_id
can be None, I don't think this is safe
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.
We check that it's not None in https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/managers/workflows.py#L1954 ?
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.
ah, right ...
args = [] | ||
args = [RuntimeContext(job_args)] | ||
kwargs: Dict[str, str] = {} | ||
if RuntimeContext is not None: | ||
args.append(RuntimeContext(job_args)) | ||
else: | ||
kwargs = job_args |
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 think the previous code should be restored with a # type: ignore[unreachable]
added to this last line, as RuntimeContext
can be None
in the tool-util package if the cwl
extra is not specified.
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 can do that, but tool parsing would fail in https://github.com/mvdbeek/galaxy/blob/c713b37c588bba50ad7d7897632e36bc16215e4d/lib/galaxy/tool_util/cwl/parser.py#L342 too ... idk if this partial support approach makes sense ?
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.
how about
diff --git a/lib/galaxy/tool_util/cwl/parser.py b/lib/galaxy/tool_util/cwl/parser.py
index 3207eea965..ac71393c61 100644
--- a/lib/galaxy/tool_util/cwl/parser.py
+++ b/lib/galaxy/tool_util/cwl/parser.py
@@ -293,6 +293,7 @@ class ExpressionToolProxy(CommandLineToolProxy):
class JobProxy:
def __init__(self, tool_proxy, input_dict, output_dict, job_directory):
+ assert RuntimeContext is not None, "cwltool is not installed, cannot run CWL jobs"
self._tool_proxy = tool_proxy
self._input_dict = input_dict
self._output_dict = output_dict
instead ?
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.
Sounds good for me. @jmchilton ?
Not going to do further work on upload1, so not fixing this.
6a6aad2
to
d178b6e
Compare
267f497
to
214bffc
Compare
214bffc
to
b757281
Compare
Haven't fixed all cases, see mypy.ini.
How to test the changes?
(Select all options that apply)
License