Skip to content

Pylint fix for invalid TOML config #4720

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Nov 13, 2021
Merged

Pylint fix for invalid TOML config #4720

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a685cf0
Adding check for dict
tanvimoharir Jul 19, 2021
48bce35
Fix crashes during toml configuration parsing
Pierre-Sassoulas Oct 29, 2021
19ede0f
Apply suggestions from code review
Pierre-Sassoulas Nov 12, 2021
04c0603
Add entry in the what's new for 2.12 and change the message
Pierre-Sassoulas Nov 13, 2021
a0745b9
Apply suggestions from code review
Pierre-Sassoulas Nov 13, 2021
22ed923
Update message name and related tests
Pierre-Sassoulas Nov 13, 2021
0fbaadc
Add followu up issue in what's new
Pierre-Sassoulas Nov 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CONTRIBUTORS.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,9 @@ contributors:

* Marcin Kurczewski (rr-): contributor

* Tanvi Moharir: contributor
- Fix for invalid toml config

* Eisuke Kawashima (e-kwsm): contributor

* Daniel van Noord (DanielNoord): contributor
Expand Down
11 changes: 11 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,17 @@ Release date: TBA

Closes #5261

* Crashes when a list is encountered in a toml configuration do not happen anymore.

Closes #4580

* A new ``bad-configuration-option-value`` checker was added that will emit for misplaced option
in pylint's top level namespace for toml configuration. Top-level dictionaries or option defined
in the wrong section will still silently not be taken into account, which is tracked in a
follow-up issue.

Follow-up in #5259

* Added new checker ``useless-with-lock`` to find incorrect usage of with statement and threading module locks.
Emitted when ``with threading.Lock():`` is used instead of ``with lock_instance:``.

Expand Down
9 changes: 9 additions & 0 deletions doc/whatsnew/2.12.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ New checkers

Closes #5208

* A new ``bad-configuration-option-value`` checker was added that will emit for misplaced option
in pylint's top level namespace for toml configuration. Top-level dictionaries or option defined
in the wrong section will still silently not be taken into account, which is tracked in a
follow-up issue.

Removed checkers
================

Expand Down Expand Up @@ -157,4 +162,8 @@ Other Changes

Closes #5216

* Crashes when a list is encountered in a toml configuration do not happen anymore.

Closes #4580

* Make yn validator case insensitive, to allow for ``True`` and ``False`` in config files.
21 changes: 16 additions & 5 deletions pylint/config/option_manager_mixin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,9 +293,8 @@ def read_config_file(self, config_file=None, verbose=None):
msg = "No config file found, using default configuration"
print(msg, file=sys.stderr)

@staticmethod
def _parse_toml(
config_file: Union[Path, str], parser: configparser.ConfigParser
self, config_file: Union[Path, str], parser: configparser.ConfigParser
) -> None:
"""Parse and handle errors of a toml configuration file."""
with open(config_file, encoding="utf-8") as fp:
Expand All @@ -305,16 +304,28 @@ def _parse_toml(
except KeyError:
return
for section, values in sections_values.items():
section_name = section.upper()
# TOML has rich types, convert values to
# strings as ConfigParser expects.
if not isinstance(values, dict):
# This class is a mixin: add_message comes from the `PyLinter` class
self.add_message( # type: ignore
"bad-configuration-option-value", line=0, args=(section, values)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably overlooking something really obvious, but how does this check whether load-plugins = [] should be in the tools.pylint section (as one of the tests does)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing directly in the tools.pylint except header (master, message controls, etc), so at top level, if it's not a dict it can't be right. We know nothing about what to expect in the configuration except that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps bad-configuration-section would be better then? For me option-value implies there is something wrong with the type of the option value (but that might be just me).

I think such a check (option value types) would be helpful anyway, if we don't already check it, so reserving bad-configuration-option-value for that checker would be better.
Putting stuff in the wrong section and giving stuff the incorrect type are different mistakes and require different messages. For the the latter we could (for example) also include the expected type in the message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I'm going to change the name and upgrade the tests.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas Nov 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

22ed923

)
continue
for option, value in values.items():
if isinstance(value, bool):
values[option] = "yes" if value else "no"
elif isinstance(value, (int, float)):
values[option] = str(value)
elif isinstance(value, list):
values[option] = ",".join(value)
parser._sections[section.upper()] = values # type: ignore
else:
values[option] = str(value)
for option, value in values.items():
try:
parser.set(section_name, option, value=value)
except configparser.NoSectionError:
parser.add_section(section_name)
parser.set(section_name, option, value=value)

def load_config_file(self):
"""Dispatch values previously read from a configuration file to each
Expand Down
6 changes: 6 additions & 0 deletions pylint/lint/pylinter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,12 @@ def _load_reporter_by_class(reporter_class: str) -> type:
"bad-plugin-value",
"Used when a bad value is used in 'load-plugins'.",
),
"E0014": (
"Incorrect setting encountered in top level configuration-section '%s' : '%s'",
"bad-configuration-option-value",
"Used when a top section value can't be parsed because it does not belong to the top section "
"or has an incorrect value type.",
),
}


Expand Down
2 changes: 2 additions & 0 deletions tests/config/functional/toml/issue_4580/correct_basic_name_group.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[tool.pylint.basic]
name-group = { "a"="b" }
2 changes: 2 additions & 0 deletions tests/config/functional/toml/issue_4580/correct_import_preferred_module.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[tool.pylint.imports]
preferred-modules = { "a"="b" }
2 changes: 2 additions & 0 deletions tests/config/functional/toml/issue_4580/empty_list.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
************* Module {abspath}
{relpath}:1:0: E0014: Incorrect setting encountered in top level configuration-section 'load-plugins' : '[]' (bad-configuration-option-value)
3 changes: 3 additions & 0 deletions tests/config/functional/toml/issue_4580/empty_list.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[tool.pylint]
# load-plugins does not belong in top level section
load-plugins = []
3 changes: 3 additions & 0 deletions tests/config/functional/toml/issue_4580/invalid_data_for_basic.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
************* Module {abspath}
{relpath}:1:0: E0014: Incorrect setting encountered in top level configuration-section 'load-plugins' : '[]' (bad-configuration-option-value)
{relpath}:1:0: E0014: Incorrect setting encountered in top level configuration-section 'disable' : 'logging-not-lazy,logging-format-interpolation' (bad-configuration-option-value)
4 changes: 4 additions & 0 deletions tests/config/functional/toml/issue_4580/invalid_data_for_basic.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[tool.pylint]
# Both disable and load-plugins do not belong in the top level section
load-plugins = []
disable = "logging-not-lazy,logging-format-interpolation"
5 changes: 5 additions & 0 deletions tests/config/functional/toml/issue_4580/invalid_data_for_import.result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"functional_append": {
"disable": [["logging-not-lazy"], ["logging-format-interpolation"]]
}
}
8 changes: 8 additions & 0 deletions tests/config/functional/toml/issue_4580/invalid_data_for_import.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Both disable and load-plugins do not belong in the imports section
# TODO This should be fixed in #5259
[tool.pylint."imports"]
disable = [
"logging-not-lazy",
"logging-format-interpolation",
]
preferred-modules = { "a"="b" }
7 changes: 7 additions & 0 deletions tests/config/functional/toml/issue_4580/rich_types.result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"functional_append": {
"disable": [["logging-not-lazy"], ["logging-format-interpolation"]]
},
"jobs": 10,
"reports": true
}
7 changes: 7 additions & 0 deletions tests/config/functional/toml/issue_4580/rich_types.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[tool.pylint."messages control"]
disable = [
"logging-not-lazy",
"logging-format-interpolation",
]
jobs = 10
reports = true
2 changes: 2 additions & 0 deletions tests/config/functional/toml/issue_4580/top_level_disable.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
************* Module {abspath}
{relpath}:1:0: E0014: Incorrect setting encountered in top level configuration-section 'disable' : 'logging-not-lazy,logging-format-interpolation' (bad-configuration-option-value)
3 changes: 3 additions & 0 deletions tests/config/functional/toml/issue_4580/top_level_disable.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[tool.pylint]
# disable does not belong in top level section
disable = "logging-not-lazy,logging-format-interpolation"