Pylint fix for invalid TOML config #4720

tanvimoharir · 2021-07-19T17:12:01Z

Steps

Add yourself to CONTRIBUTORS if you are a new contributor.
Add a ChangeLog entry describing what your PR does.
If it's a new feature or an important bug fix, add a What's New entry in
doc/whatsnew/<current release.rst>.
Write a good description on what the PR does.

Description

This is a fix for handling bad or invalid toml configuration in pyproject.toml

Type of Changes

	Type
✓	🐛 Bug fix
✓	✨ New feature
✓	🔨 Refactoring
✓	📜 Docs

Related Issue

Closes #4580

coveralls · 2021-07-19T17:22:01Z

Pull Request Test Coverage Report for Build 1456420246

8 of 11 (72.73%) changed or added relevant lines in 1 file are covered.
3 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.02%) to 93.386%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/config/option_manager_mixin.py	8	11	72.73%

Files with Coverage Reduction	New Missed Lines	%
pylint/extensions/docparams.py	3	97.74%

Totals
Change from base Build 1454801709:	-0.02%
Covered Lines:	13879
Relevant Lines:	14862

💛 - Coveralls

Pierre-Sassoulas

Could you add a test with the failing toml, please ? :)

tanvimoharir · 2021-07-20T07:33:01Z

Could you add a test with the failing toml, please ? :)

Yes I will do that, I also need to accommodate the other failed case which was AttributeError: 'DynamicInlineTableDict' object has no attribute 'find'

tanvimoharir · 2021-07-21T14:37:49Z

@Pierre-Sassoulas I added three tests, one for empty list and two more for the other two cases mentioned in #4580
However for the case for AttributeError: 'DynamicInlineTableDict' object has no attribute 'find' its still crashing.

tanvimoharir · 2021-07-21T14:51:37Z

tests/test_config.py

+def test_toml_with_invalid_data_for_imports(tmp_path):
+    # This would test config with invalid data for imports section
+    # refer #4580
+    config_file = tmp_path / "pyproject.toml"
+    config_file.write_text(
+        """
+[tool.pylint."imports"]
+preferred-modules = { "a"="b" }
+"""
+    )
+    with pytest.raises(AttributeError):
+        check_configuration_file_reader(config_file)
+
+
+def test_toml_with_invalid_data_for_basic(tmp_path):
+    # This would test config with invalid data for basic section
+    # refer #4580
+    config_file = tmp_path / "pyproject.toml"
+    config_file.write_text(
+        """
+[tool.pylint."basic"]
+name-group = { "a"="b" }
+"""


Per my other comment these two cases are still crashing.

Pierre-Sassoulas

The tests looks nice, do you need help to fix the use case that is failing ?

tanvimoharir · 2021-07-22T09:41:16Z

The tests looks nice, do you need help to fix the use case that is failing ?

I have attempted a fix.

tanvimoharir · 2021-07-24T16:41:05Z

The tests looks nice, do you need help to fix the use case that is failing ?

Yes, it would be great if you help me with this. I have tried few things and they dont seem to be working although the tests pass when I executed locally.

Pierre-Sassoulas · 2021-07-25T13:44:31Z

I think we need to define better what is expected in the pyproject.toml for pylint. The current crash might be due to disable = "logging-not-lazy,logging-format-interpolation" not being inside "MESSAGES CONTROL". But we'd need to have proper error message when something is misplaced, because right now it's not very convenient and it's just crashing when the configuration is not correct.

I guess the expected configuration for the failing test should be:

[tool.pylint]
    [tool.pylint."messages control"]
        disable = "logging-not-lazy,logging-format-interpolation"
    [tool.pylint."master"]
        load-plugins = []

(Correct me if I'm wrong, I'm no toml expert. But when using the current conf instead of crashing we should have an error message like:

Disable is not a valid section for pylint, did you mean '[tool.pylint."messages control"]  disable = ...' ?

tanvimoharir · 2021-07-25T17:47:23Z

I think we need to define better what is expected in the pyproject.toml for pylint. The current crash might be due to disable = "logging-not-lazy,logging-format-interpolation" not being inside "MESSAGES CONTROL". But we'd need to have proper error message when something is misplaced, because right now it's not very convenient and it's just crashing when the configuration is not correct.

I guess the expected configuration for the failing test should be:
[tool.pylint]
    [tool.pylint."messages control"]
        disable = "logging-not-lazy,logging-format-interpolation"
    [tool.pylint."master"]
        load-plugins = []
(Correct me if I'm wrong, I'm no toml expert. But when using the current conf instead of crashing we should have an error message like:
Disable is not a valid section for pylint, did you mean '[tool.pylint."messages control"]  disable = ...' ?

Thanks @Pierre-Sassoulas I have updated with the configuration you have specified here. I will look more into TOML types since I'm not that familiar with it.

tanvimoharir · 2021-08-30T16:33:28Z

@Pierre-Sassoulas Could you help me proceed with this ? I'm not able to understand it clearly.

Pierre-Sassoulas · 2021-09-01T20:08:52Z

Hey sorry for the time it took to answer this. It look like the configuration from the toml is not taken into account like before as some integration tests are failing. If I were you I would add unit test on the TOML configuration in order to understand what is going wrong in the integrations tests first. Do not hesitate to ask if there is something specific that is blocking you.

tanvimoharir · 2021-09-05T12:56:32Z

Hey sorry for the time it took to answer this. It look like the configuration from the toml is not taken into account like before as some integration tests are failing. If I were you I would add unit test on the TOML configuration in order to understand what is going wrong in the integrations tests first. Do not hesitate to ask if there is something specific that is blocking you.

The current failures which i see through via pipeline details are because of the tests which I added.
Do you think I should remove those?
Let me know if I'm missing something.

Handle https://github.com/PyCQA/pylint/pull/4720\#discussion_r742853160

Pierre-Sassoulas

This is ready for review following #5276 and #5277.

DanielNoord

Change (like previously) looks good! I like how we are now handling most errors while still continuing.

Mostly some comments about the tests.

tests/config/issue_4580/invalid_data_for_import.toml

tests/config/issue_4580/empty_list.toml

tests/config/issue_4580/invalid_data_for_basic.toml

tests/config/issue_4580/top_level_disable.toml

tests/config/test_config_pyproject_toml.py

pylint/lint/pylinter.py

tests/config/test_config_pyproject_toml.py

pylint/config/option_manager_mixin.py

pylint/lint/pylinter.py

tests/config/test_functional_toml_parsing.py

Pierre-Sassoulas · 2021-11-08T12:17:56Z

tests/config/functional/issue_4580/invalid_data_for_import.toml

@@ -0,0 +1,7 @@
+# Both disable and load-plugins do not belong in the imports section


Notice how we do not detect anything here, but we should.

Necessary prior to #4720

Add test for current pyproject.toml issues Add a 'bad-configuration-option-value' message for bad toml configuration We do not catch all the problem in toml because we don't know what is expected so we can't recommend. See pylint-dev#5259 We can detect bad top level option when reading the toml

Pierre-Sassoulas

Following #5287 this became a more manageable change, it's ready for review @DanielNoord :)

tests/config/functional/toml/issue_4580/invalid_data_for_import.toml

DanielNoord

I don't really understand how the new checker works, or at least: how its description fits the single case where it can be emitted in option_manager_mixin.

ChangeLog

pylint/config/option_manager_mixin.py

DanielNoord · 2021-11-12T21:51:37Z

pylint/config/option_manager_mixin.py

+            if not isinstance(values, dict):
+                # This class is a mixin the add_message come from the pylinter
+                self.add_message(  # type: ignore
+                    "bad-configuration-option-value", line=0, args=(section, values)


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)?

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.

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.

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

pylint/lint/pylinter.py

Co-authored-by: Daniël van Noord <[email protected]>

DanielNoord

I understand what the message is checking now. I think renaming and rewording the description might help future confusion. Rest looks good to go!

doc/whatsnew/2.12.rst

DanielNoord · 2021-11-13T08:40:36Z

pylint/config/option_manager_mixin.py

+            if not isinstance(values, dict):
+                # This class is a mixin the add_message come from the pylinter
+                self.add_message(  # type: ignore
+                    "bad-configuration-option-value", line=0, args=(section, values)


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.

pylint/lint/pylinter.py

Co-authored-by: Daniël van Noord <[email protected]>

tanvimoharir changed the title ~~Adding check for dict~~ Pylint fix for invalid TOML config Jul 19, 2021

Pierre-Sassoulas added the Crash 💥 A bug that makes pylint crash label Jul 19, 2021

Pierre-Sassoulas added this to the 2.9.4 milestone Jul 19, 2021

Pierre-Sassoulas reviewed Jul 19, 2021

View reviewed changes

Pierre-Sassoulas modified the milestones: 2.9.4, 2.9.5 Jul 19, 2021

Pierre-Sassoulas modified the milestones: 2.9.5, 2.9.6 Jul 21, 2021

Pierre-Sassoulas added the Configuration Related to configuration label Jul 21, 2021

tanvimoharir commented Jul 21, 2021

View reviewed changes

tanvimoharir requested a review from Pierre-Sassoulas July 21, 2021 15:51

Pierre-Sassoulas reviewed Jul 21, 2021

View reviewed changes

Pierre-Sassoulas force-pushed the invalid-toml-config branch from 9297688 to f269c4d Compare July 25, 2021 13:47

Pierre-Sassoulas mentioned this pull request Jul 25, 2021

Documentation page for configuration file #3181

Closed

Pierre-Sassoulas linked an issue Jul 25, 2021 that may be closed by this pull request

Pylint crashes for pyproject.toml configuration mixed with another tool #4746

Closed

Pierre-Sassoulas modified the milestones: 2.9.6, 2.10.1 Jul 25, 2021

Pierre-Sassoulas modified the milestones: 2.10.2, 2.10.x Aug 21, 2021

Pierre-Sassoulas referenced this pull request Nov 8, 2021

Create a function for toml parsing

abd31cd

Handle https://github.com/PyCQA/pylint/pull/4720\#discussion_r742853160

Pierre-Sassoulas force-pushed the invalid-toml-config branch from d5b765d to 946adb2 Compare November 8, 2021 09:16

Pierre-Sassoulas reviewed Nov 8, 2021

View reviewed changes

Pierre-Sassoulas marked this pull request as ready for review November 8, 2021 09:18

DanielNoord reviewed Nov 8, 2021

View reviewed changes

Pierre-Sassoulas force-pushed the invalid-toml-config branch from 946adb2 to 9e798d1 Compare November 8, 2021 09:38

DanielNoord reviewed Nov 8, 2021

View reviewed changes

pylint/lint/pylinter.py Outdated Show resolved Hide resolved

DanielNoord reviewed Nov 8, 2021

View reviewed changes

tests/config/test_functional_toml_parsing.py Outdated Show resolved Hide resolved

Pierre-Sassoulas reviewed Nov 8, 2021

View reviewed changes

Pierre-Sassoulas mentioned this pull request Nov 9, 2021

Simplify test_can_read_toml_env_variable to work like other tests #5284

Merged

Pierre-Sassoulas added a commit that referenced this pull request Nov 9, 2021

Refactor 'check_configuration_file_reader' so we can assert other files

37390e2

Necessary prior to #4720

Pierre-Sassoulas added a commit that referenced this pull request Nov 9, 2021

Refactor 'check_configuration_file_reader' so we can assert other files

3e87888

Necessary prior to #4720

Pierre-Sassoulas added a commit that referenced this pull request Nov 9, 2021

Refactor 'check_configuration_file_reader' so we can assert other files

98e208b

Necessary prior to #4720

Pierre-Sassoulas added a commit that referenced this pull request Nov 9, 2021

Refactor 'check_configuration_file_reader' so we can assert other files

34eeaf0

Necessary prior to #4720

Pierre-Sassoulas mentioned this pull request Nov 10, 2021

Create a framework of functional tests for configuration files #5287

Merged

Pierre-Sassoulas force-pushed the invalid-toml-config branch from 700b66b to 953050d Compare November 10, 2021 18:43

tanvimoharir and others added 2 commits November 12, 2021 22:29

Adding check for dict

a685cf0

Pierre-Sassoulas force-pushed the invalid-toml-config branch from 953050d to 48bce35 Compare November 12, 2021 21:34

Pierre-Sassoulas reviewed Nov 12, 2021

View reviewed changes

Pierre-Sassoulas requested a review from DanielNoord November 12, 2021 21:36

Pierre-Sassoulas reviewed Nov 12, 2021

View reviewed changes

tests/config/functional/toml/issue_4580/invalid_data_for_import.toml Show resolved Hide resolved

DanielNoord reviewed Nov 12, 2021

View reviewed changes

Pierre-Sassoulas and others added 2 commits November 12, 2021 23:50

Apply suggestions from code review

19ede0f

Co-authored-by: Daniël van Noord <[email protected]>

Add entry in the what's new for 2.12 and change the message

04c0603

DanielNoord reviewed Nov 13, 2021

View reviewed changes

Pierre-Sassoulas and others added 3 commits November 13, 2021 10:14

Apply suggestions from code review

a0745b9

Co-authored-by: Daniël van Noord <[email protected]>

Update message name and related tests

22ed923

Add followu up issue in what's new

0fbaadc

Pierre-Sassoulas merged commit 1a20bc5 into pylint-dev:main Nov 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pylint fix for invalid TOML config #4720

Pylint fix for invalid TOML config #4720

tanvimoharir commented Jul 19, 2021 •

edited

Loading

coveralls commented Jul 19, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

tanvimoharir commented Jul 20, 2021

tanvimoharir commented Jul 21, 2021

tanvimoharir Jul 21, 2021

Pierre-Sassoulas left a comment

tanvimoharir commented Jul 22, 2021

tanvimoharir commented Jul 24, 2021

Pierre-Sassoulas commented Jul 25, 2021

tanvimoharir commented Jul 25, 2021

tanvimoharir commented Aug 30, 2021

Pierre-Sassoulas commented Sep 1, 2021

tanvimoharir commented Sep 5, 2021

Pierre-Sassoulas left a comment •

edited

Loading

DanielNoord left a comment

Pierre-Sassoulas Nov 8, 2021

Pierre-Sassoulas left a comment

DanielNoord left a comment

DanielNoord Nov 12, 2021

Pierre-Sassoulas Nov 12, 2021

DanielNoord Nov 13, 2021

Pierre-Sassoulas Nov 13, 2021

Pierre-Sassoulas Nov 13, 2021 •

edited

Loading

DanielNoord left a comment

DanielNoord Nov 13, 2021

		@@ -0,0 +1,7 @@
		# Both disable and load-plugins do not belong in the imports section

Pylint fix for invalid TOML config #4720

Pylint fix for invalid TOML config #4720

Conversation

tanvimoharir commented Jul 19, 2021 • edited Loading

Steps

Description

Type of Changes

Related Issue

coveralls commented Jul 19, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1456420246

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

tanvimoharir commented Jul 20, 2021

tanvimoharir commented Jul 21, 2021

tanvimoharir Jul 21, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

tanvimoharir commented Jul 22, 2021

tanvimoharir commented Jul 24, 2021

Pierre-Sassoulas commented Jul 25, 2021

tanvimoharir commented Jul 25, 2021

tanvimoharir commented Aug 30, 2021

Pierre-Sassoulas commented Sep 1, 2021

tanvimoharir commented Sep 5, 2021

Pierre-Sassoulas left a comment • edited Loading

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas Nov 8, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord Nov 12, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Nov 12, 2021

Choose a reason for hiding this comment

DanielNoord Nov 13, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Nov 13, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Nov 13, 2021 • edited Loading

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord Nov 13, 2021

Choose a reason for hiding this comment

tanvimoharir commented Jul 19, 2021 •

edited

Loading

coveralls commented Jul 19, 2021 •

edited

Loading

Pierre-Sassoulas left a comment •

edited

Loading

Pierre-Sassoulas Nov 13, 2021 •

edited

Loading