-
Notifications
You must be signed in to change notification settings - Fork 152
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
Refactor Command.run() #2467
Refactor Command.run() #2467
Conversation
Umm wow. @schaefi can we have this staged to run image build tests before I approve and merge this? |
yes I will stage this. It's a big change |
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.
Thanks, this is a nice change 👍
I will run the staging builds with it to check if anything breaks. Other than that I only have one question on the use of assert
I submitted to Staging, let's see what happens :) |
@dcermak The changes works except for distros with py <3.8, there it ends with
This is easy to fix and as long as it's easy to stay compatible we should do this. Thus can you please add -from typing import TYPE_CHECKING, Dict, Literal, Union, overload
+import sys
+from typing import TYPE_CHECKING, Dict, Union, overload
+
+if sys.version_info >= (3, 8):
+ from typing import Literal # pragma: no cover
+else: # pragma: no cover
+ from typing_extensions import Literal # pragma: no cover This will fix it. Thanks |
Command.run() currently has a bit of a confusing behavior: if raise_on_error is False and the executable is not found, then a weird CommandT is returned (return code is -1 and stdout+stderr is None). This makes it possible to hanlde command not found errors separately, but it makes that needlessly verbose. So instead, let's just return None in *this* special case. That in turn uncovered, that in most cases when we set `raise_on_error=True`, we actually want an error if the command is not present but no error if the command fails to execute (e.g. because it returns -1 if you run `$cmd --version` 🙄). Hence we introduce the flag `raise_on_command_not_found`, which causes an exception to be raised if the command is not found. This makes it independent of the `raise_on_error` flag. Additionally, we add a small optimization: if command starts with /, then we assume it's a full path and we omit the call to which (and just check whether it exists). These changes require additional tests and a few assertions in places where mypy cannot infer that a certain value is of the correct type.
58253dc
to
0cb6a3d
Compare
Done. I have also added a test run for both python 3.8 & 3.7, as we'll otherwise get more of these failures in the future. |
Due to changes in commit fb69627
@dcermak I added a commit to skip the 3.7 unit test runs and will create a new Staging build once the green checkmark is back. I'd like to get this one in and wanted to zip a new Staging build from a clean state, hope that's ok As we cherry-picked a fix to master (for a release to ALP) I also need to fix the compliance check as I think we have to allow-empty there... that's why I add to your work |
IIRC, we're also dropping Python 3.8 since our new floor will be Python 3.9? |
we said so but there are a few code parts that differentiate on the import for the typing vs. typing_extensions and the boundary here is py3.8. So for the unit test matrix I agree with Dan that we should keep 3.8 in when we have code for it. I don't want to delete the code that differentiates on the typing vs typing_extensions because that also keeps it functional for <3.8 even if that is something we don't care so much. It's very simple to keep that and there is no point in removing imho |
If we cherry-pick from main to master the compliance check will notice that a commit already exists. This is not an error and we can allow to continue the picking via --allow-empty
Creating a new Staging build... |
How about we just run mypy for python 3.7? I don't like the idea of having code that we don't test in production, but that will give us at least errors on these kind of import issues. |
sounds good to me 👍 |
Command.run()
currently has a bit of a confusing behavior: ifraise_on_error
isFalse
and the executable is not found, then a weirdCommandT
is returned (return code is -1 and stdout+stderr is None). This makes it possible to handle command not found errors separately, but it makes that needlessly verbose. So instead, let's just returnNone
in this special case.That in turn uncovered, that in most cases when we set
raise_on_error=True
, we actually want an error if the command is not present but no error if the command fails to execute (e.g. because it returns -1 if you run$cmd --version
🙄). Hence we introduce the flagraise_on_command_not_found
, which causes an exception to be raised if the command is not found. This makes it independent of theraise_on_error
flag.Additionally, we add a small optimization: if command starts with /, then we assume it's a full path and we omit the call to which (and just check whether it exists).
These changes require additional tests and a few assertions in places where mypy cannot infer that a certain value is of the correct type.
Also, add type hints in a few (mostly random) places.