-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
return an error if the explicitly specified config is missing #1498
Conversation
src/flake8/main/application.py
Outdated
assert self.options is not None | ||
if self.options.exit_zero: |
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 is unrelated to your change. Please revert it
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.
Without changing the two assertions, the output is:
$ flake8 --config foo.cfg t.py
There was a critical error during execution of Flake8:
The specified config file does not exist: foo.cfg
Traceback (most recent call last):
File "/home/rkm/dev/pycqa/flake8/venv/bin/flake8", line 33, in <module>
sys.exit(load_entry_point('flake8', 'console_scripts', 'flake8')())
File "/home/rkm/dev/pycqa/flake8/src/flake8/main/cli.py", line 22, in main
app.run(argv)
File "/home/rkm/dev/pycqa/flake8/src/flake8/main/application.py", line 395, in run
assert self.options is not None
AssertionError
Compared to simply:
$ flake8 --config foo.cfg t.py
There was a critical error during execution of Flake8:
The specified config file does not exist: foo.cfg
To me the traceback seems misleading, since it doesn't point to the actual call site. If that's acceptable then I can re-add the assertions, otherwise let me know if you have a better solution in mind.
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 this should be
def exit_code(self) -> int:
"""Return the program exit code."""
if self.catastrophic_failure:
return 1
assert self.options is not None
if self.options.exit_zero:
return 0
else:
return int(self.result_count > 0)
eventually I want to factor away the Application
class since the lateinit nullable attributes pose a lot of problems and we have to side-step the typechecker a lot
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.
actually it's slightly more involved -- here's a fix for that:
diff --git a/src/flake8/main/application.py b/src/flake8/main/application.py
index ee46f6a..1894881 100644
--- a/src/flake8/main/application.py
+++ b/src/flake8/main/application.py
@@ -115,10 +115,13 @@ class Application:
def exit_code(self) -> int:
"""Return the program exit code."""
- if self.options and self.options.exit_zero:
- return int(self.catastrophic_failure)
+ if self.catastrophic_failure:
+ return 1
+ assert self.options is not None
+ if self.options.exit_zero:
+ return 0
else:
- return int((self.result_count > 0) or self.catastrophic_failure)
+ return int(self.result_count > 0)
def find_plugins(
self,
@@ -390,6 +393,7 @@ class Application:
except exceptions.EarlyQuit:
self.catastrophic_failure = True
print("... stopped while processing files")
-
- if self.options and self.options.count:
- print(self.result_count)
+ else:
+ assert self.options is not None
+ if self.options.count:
+ print(self.result_count)
actually, I think this is broken on main
so I'll split it out to a separate patch
src/flake8/options/config.py
Outdated
@@ -60,6 +61,10 @@ def load_config( | |||
|
|||
cfg = configparser.RawConfigParser() | |||
if config is not None: | |||
if not os.path.isfile(config): | |||
raise exceptions.ExecutionError( | |||
"The specified config file does not exist" |
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.
Why wouldn't you include the config value in the exception?
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.
have added this now
We have not documented that we would raise an error if the configuration file doesn't exist and if we wanted to split hairs, this is backwards incompatible behaviour meaning we couldn't easily just accept this at the moment |
Thanks for reviewing. I hadn't considered there would be any worries about backwards incompatibility. Feel free to close this if you think that's a blocker. If so then could we at least update the docs to indicate the existing behaviour? |
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.
while this ~technically breaks backward compatibility -- I think this is a move in the right direction and I'm ok with this being a new error
just needs a slight rebase though 👍
b033921
to
d948169
Compare
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.
fixes #1497