Skip to content
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

Removed code snippet in the error message for config file parsing errors #927

Conversation

sushimon
Copy link
Contributor

@sushimon sushimon commented Jul 12, 2023

Motivation and Context

From #920, it is apparent that pylint does not accurately display the lines containing the error for config file parsing errors. This PR aims to prevent the reporter from displaying the incorrect lines as the source of the error and simply show no code instead.

Your Changes

Description:

  • Added certain config file parsing error ids to NO_SNIPPET which prevents the reporter from displaying incorrect lines as the sources of error.
  • Generalized some test cases to include the newly blacklisted config file parsing errors.
  • Added a new test case to verify that no snippet was built for the blacklisted config file parsing errors.
  • Updated _override_config to add E0015 to the reporter instead of printing to stderr.

Type of change (select all that apply):

  • Bug fix (non-breaking change which fixes an issue)
  • Test update (change that modifies or updates tests only)

Testing

  • Ran the test suite and verified they all passed.
  • Ran the modified and new tests and verified they passed.

Questions and Comments (if applicable)

  • Did not include the F0011 error in the updated tests since it seems to cause all other config file errors to be excluded. I ran the updated test_config_parsing_errors but included F0011 in CONFIG_ERRORS_TO_CHECK, but after debugging, it seems to not report the W0012 and R0022 even though those errors are in the config file. Maybe since it's a "fatal" error, it stops pylint from checking for other errors?
  • F0011 and R0022 were the only errors that I was able to reproduce. The other errors whose id started with "00" were probably either command line or inline errors.
  • E0015 was not showing up in the reporter because the message was being printed to stderr instead inside of _override_config. I've updated it to now add the message to the reporter.

Checklist

  • I have performed a self-review of my own code.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.
  • I have added tests for my changes.
  • I have updated the CHANGELOG.md file.

@coveralls
Copy link
Collaborator

coveralls commented Jul 12, 2023

Pull Request Test Coverage Report for Build 5549911587

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

Totals Coverage Status
Change from base Build 5502983351: 4.1%
Covered Lines: 3053
Relevant Lines: 3245

💛 - Coveralls

Copy link
Contributor

@david-yz-liu david-yz-liu left a comment

Choose a reason for hiding this comment

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

@sushimon good work!

  • For testing, you should create a separate test case and file for the config parse error.
  • I think you missed a fewer errors that aren't in the "W" category: E0015 and R0022 at least.

@sushimon
Copy link
Contributor Author

@sushimon good work!

  • For testing, you should create a separate test case and file for the config parse error.
  • I think you missed a fewer errors that aren't in the "W" category: E0015 and R0022 at least.

In the latest commit I've:

  • Created a separate test case for F0011. I've structured it so that non-fatal errors can be checked inside test_config_parsing_errors and fatal errors will have to be checked separately in their own individual test case and config file.
  • R0022 was already being checked in the very first commit but E0015 has been added.

@sushimon sushimon force-pushed the remove-snippet-from-config-parsing-errors branch from 9c07ced to 1e91207 Compare July 14, 2023 02:44
@david-yz-liu david-yz-liu merged commit 2109a69 into pyta-uoft:master Jul 14, 2023
@sushimon sushimon deleted the remove-snippet-from-config-parsing-errors branch July 31, 2023 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants