-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Output a warning to stderr when an invalid key is read from an INI config file #7286
Conversation
So now, the output looks like this:
|
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 for getting this started it's a good idea
the validation call has to happen after all plugins are registered
we might need a additional declaration of required plugins to know all the options or a way to opt out of this for projects that declare options of pytest plugins which are not always installed
src/_pytest/config/__init__.py
Outdated
@@ -1072,6 +1073,14 @@ def _checkversion(self): | |||
) | |||
) | |||
|
|||
def _validatekeys(self): | |||
for key in self._get_unknown_ini_keys(): | |||
sys.stderr.write("WARNING: unknown config ini key: {}\n".format(key)) |
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 wonder if we should use real python warning here and set the file to the configfile/the line to the line of the key
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 used sys.stderr
becauase it looks like the rest of this file does ( see https://github.com/pytest-dev/pytest/blob/master/src/_pytest/config/__init__.py#L1228 for example ). I don't mind changing it though
sorry @RonnyPfannschmidt I didn't see your review and added in the |
@RonnyPfannschmidt - I think I placed the validation after plugin consideration now. |
Personally, I think we should keep with the file and use |
Created a new issue to track the migration to the warnings module. I think this PR is good for another look, @RonnyPfannschmidt |
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.
Looking good now
The changelog fragments is empty ,
And a second follow up issue we need is the list of the required plugins
Sorry about that @RonnyPfannschmidt, I could have sworn I wrote the change log entry. It's added in now. I'll create the second follow up issue tonight |
@gnikonorov it looks like a good old `git add´ before adding content, most of us have been there and review has our backs for that ^^ |
Most definitely @RonnyPfannschmidt |
we have no automated merge after green, its easy to miss/fail a detail, so merges are done by hand |
That makes sense, thanks for explaining @RonnyPfannschmidt ! |
Implementation of #6856.
Changes:
INI
key is read from a config ini file.--strict-config
which forces warnings to become errors