-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
@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 π |
Pull Request Test Coverage Report for Build 1490150360
π - Coveralls |
There was a problem hiding this 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'" |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
216f94f
to
ca8a6e4
Compare
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 ? |
There was a problem hiding this 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.
I think we should leave this issue open until |
3a6ddc9
to
50ce066
Compare
Type of Changes
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