Skip to content

Regression test for 3181, add documentation for toml configuration#5365

Merged
Pierre-Sassoulas merged 3 commits into
mainfrom
regression-test-for-3181
Nov 22, 2021
Merged

Regression test for 3181, add documentation for toml configuration#5365
Pierre-Sassoulas merged 3 commits into
mainfrom
regression-test-for-3181

Conversation

@Pierre-Sassoulas
Copy link
Copy Markdown
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Nov 22, 2021

Type of Changes

Type
🐛 Bug fix
🔨 Refactoring
📜 Docs

Description

This add two regression tests for #3181, fix a crash when we can't decode the TOML file, and modify the configuration framework to be able to handle the expected exit code (it can be 1 for fatal too).

Refer to #3181

@Pierre-Sassoulas Pierre-Sassoulas added Configuration Related to configuration Documentation 📗 Crash 💥 A bug that makes pylint crash labels Nov 22, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Nov 22, 2021
@Pierre-Sassoulas
Copy link
Copy Markdown
Member Author

@DanielNoord sorry for adding another issue in 2.12, but it felt like a really small doc fix on a popular high priority issue, then I added regression and triggered a crash with an invalid toml conf, and one thing leading to another I had to change the test framework to handle "fatal" exit code 😄

@coveralls
Copy link
Copy Markdown

coveralls commented Nov 22, 2021

Pull Request Test Coverage Report for Build 1490150360

  • 26 of 26 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 93.443%

Totals Coverage Status
Change from base Build 1487596931: 0.007%
Covered Lines: 13923
Relevant Lines: 14900

💛 - Coveralls

Copy link
Copy Markdown
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

I have no problem with adding this to 2.12. Seems to include some good changes again!

However, I think this code is becoming quite difficult to understand when you're a first time contributor to pylint. I think we might want to create a short section in the Contributing section of our docs on how this part of the test suite works and how new tests should be added.
Once we finish the toml configuration parsing we shouldn't have to touch this much in the future, so documenting our work will be useful for ourselves as well.

Also I don't think this should close the issue that's linked. The documentation page that is linked to in the OP of that issue is still quite minimal imo. Once again, let's work on that after argparse, but I don't think the changes in this PR satisfy the requirements of that issue.

Comment thread pylint/config/option_manager_mixin.py Outdated
Comment thread pylint/testutils/configuration_test.py Outdated
exit_code = 0
msg = (
"we expect a single file of the form "
"'filename_dot_expected_error_code_dot_out.32.out'"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this name is a bit difficult to interpret with the dot. I read this as expecting a file with the form of filename.bad-configuration-section.out.32.out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread pylint/testutils/configuration_test.py Outdated

def get_related_files(
tested_configuration_file: str, suffix_filter: str
) -> List[Path]:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's add a docstring here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread tests/testutils/test_configuration_test.py
@Pierre-Sassoulas
Copy link
Copy Markdown
Member Author

but I don't think the changes in this PR satisfy the requirements of that issue.

Yes this is quite minimal, but we don't have a lot of documentation for pylintrc either (well we can generate one) and we might want to change the top level section later so it feel like it might be a lot of work to do a full documentation on the top level headers. Maybe we should tell to generate the pylintrc and look at the top level header there ?

Copy link
Copy Markdown
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

Small spelling changes here and there.

I think config-parse-error is a little more descriptive and doesn't make the name too long, but I'll let you decide here.

Comment thread pylint/config/option_manager_mixin.py Outdated
Comment thread pylint/lint/pylinter.py Outdated
Comment thread pylint/testutils/configuration_test.py Outdated
Comment thread tests/testutils/test_configuration_test.py
@DanielNoord
Copy link
Copy Markdown
Collaborator

Yes this is quite minimal, but we don't have a lot of documentation for pylintrc either (well we can generate one) and we might want to change the top level section later so it feel like it might be a lot of work to do a full documentation on the top level headers. Maybe we should tell to generate the pylintrc and look at the top level header there ?

I think we should leave this issue open until argparse and see what that offers us in terms of automatic configuration file generation and automatic configuration documentation generation.
I know that right now the auto-generated pyltinrc doesn't include all possible options (see #5154). So that would not help for all questions. Furthermore, I think there is also a discussion to be had about whether we should suggest to use pylintrc or pyproject.toml for new users.
These are all things to consider when doing a good and full update of the docs, which I think the issue asks for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Configuration Related to configuration Crash 💥 A bug that makes pylint crash Documentation 📗

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants