Skip to content

Comments

Output a warning to stderr when an invalid key is read from an INI config file#7286

Merged
RonnyPfannschmidt merged 6 commits intopytest-dev:masterfrom
gnikonorov:issue_6856
Jun 2, 2020
Merged

Output a warning to stderr when an invalid key is read from an INI config file#7286
RonnyPfannschmidt merged 6 commits intopytest-dev:masterfrom
gnikonorov:issue_6856

Conversation

@gnikonorov
Copy link
Member

@gnikonorov gnikonorov commented May 31, 2020 •
edited
Loading

Implementation of #6856.
Changes:

  • Show a warning when an invalid INI key is read from a config ini file.
  • Add the --strict-config which forces warnings to become errors

@gnikonorov
Copy link
Member Author

gnikonorov commented May 31, 2020

So now, the output looks like this:

============================================================================================================ 1 passed in 0.02s =============================================================================================================
(.venv) gnikonorov:~/pytest/gleb_test$ python3 -m pytest test_change.py
WARNING: unknown config ini key: superbadkey
=========================================================================================================== test session starts ============================================================================================================
platform linux -- Python 3.6.9, pytest-5.4.1.dev381+geef4f87e7.d20200531, py-1.8.1, pluggy-0.13.1
rootdir: /home/gnikonorov/pytest/gleb_test, inifile: pytest.ini
plugins: hypothesis-5.15.1
collected 1 item

test_change.py .                                                                                                                                                                                                                     [100%]

============================================================================================================ 1 passed in 0.02s =============================================================================================================
(.venv) gnikonorov:~/pytest/gleb_test$

RonnyPfannschmidt
RonnyPfannschmidt requested changes May 31, 2020
View reviewed changes
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a 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

def _validatekeys(self):
for key in self._get_unknown_ini_keys():
sys.stderr.write("WARNING: unknown config ini key: {}\n".format(key))
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt May 31, 2020

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

Copy link
Member Author

@gnikonorov gnikonorov May 31, 2020

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

@gnikonorov
Copy link
Member Author

gnikonorov commented May 31, 2020

sorry @RonnyPfannschmidt I didn't see your review and added in the --strict-config flag the issue requestor mentioned. It's late here, I'll update this PR again tomorrow regarding your comments

RonnyPfannschmidt
RonnyPfannschmidt reviewed May 31, 2020
View reviewed changes
@gnikonorov
Copy link
Member Author

gnikonorov commented May 31, 2020

@RonnyPfannschmidt - I think I placed the validation after plugin consideration now.
I guess we just need to resolve #7286 (comment) now

@gnikonorov
Copy link
Member Author

gnikonorov commented May 31, 2020

Personally, I think we should keep with the file and use sys.stderr for consistency, and maybe open an issue to track refactoring all sys.stderr calls into warning. Thoughts @RonnyPfannschmidt ?

@gnikonorov gnikonorov mentioned this pull request Jun 2, 2020
Closed
@gnikonorov
Copy link
Member Author

gnikonorov commented Jun 2, 2020 •
edited
Loading

Created a new issue to track the migration to the warnings module. I think this PR is good for another look, @RonnyPfannschmidt

RonnyPfannschmidt
RonnyPfannschmidt requested changes Jun 2, 2020
View reviewed changes
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a 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

@gnikonorov
Copy link
Member Author

gnikonorov commented Jun 2, 2020

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

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt commented Jun 2, 2020

@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 ^^

@gnikonorov
Copy link
Member Author

gnikonorov commented Jun 2, 2020

Most definitely @RonnyPfannschmidt
Does the pipeline not auto merge? Seems like it’s hanging on all green

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt commented Jun 2, 2020

we have no automated merge after green, its easy to miss/fail a detail, so merges are done by hand

@gnikonorov
Copy link
Member Author

gnikonorov commented Jun 2, 2020

That makes sense, thanks for explaining @RonnyPfannschmidt !

@RonnyPfannschmidt RonnyPfannschmidt merged commit 8faf1e8 into pytest-dev:master Jun 2, 2020
This was referenced Jun 2, 2020
Closed
Merged
@gnikonorov gnikonorov mentioned this pull request Jun 18, 2020
Merged
@The-Compiler The-Compiler mentioned this pull request Jul 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gnikonorov @RonnyPfannschmidt