Skip to content

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

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

Merged
merged 3 commits into from
Nov 22, 2021

Conversation

Pierre-Sassoulas
Copy link
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
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

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
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.

exit_code = 0
msg = (
"we expect a single file of the form "
"'filename_dot_expected_error_code_dot_out.32.out'"
Copy link
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
Member Author

Choose a reason for hiding this comment

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


def get_related_files(
tested_configuration_file: str, suffix_filter: str
) -> List[Path]:
Copy link
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
Member Author

Choose a reason for hiding this comment

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

@Pierre-Sassoulas
Copy link
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
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.

@DanielNoord
Copy link
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