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

Fixed bug in how PythonTA reports error messages when parsing config files #920

Merged

Conversation

sushimon
Copy link
Contributor

@sushimon sushimon commented Jun 27, 2023

Motivation and Context

When running check_all or check_errors and using a config file that contains an unknown option value (e.g. invalid message code in the "disable" section), PythonTA crashes and no report is generated. This PR aims to prevent PythonTA from crashing in this manner.

Your Changes

Description:

  • Updated the callback function on_set_current_module to infer a filepath if none was provided.
  • Added tests to verify that the bug no longer occurs.
  • Updated the CHANGELOG.md file to document this bugfix.
  • Fixed errors and bugs in how the JSONReporter, ColorReporter, and PlainReporter print messages and added test cases to verify they correctly print/display the desired messages.

Type of change (select all that apply):

  • Bug fix (non-breaking change which fixes an issue)

Testing

  • Ran the test suite and verified the tests passed
  • Ran the newly added tests and verified they've passed as well
  • Ran check_all using all four checkers and verified they all printed the expected reports.

Questions and Comments (if applicable)

  • When testing, I initially wanted to use the JSONReporter so I can easily check whether the target message id (W0012) was being reported when using check_all. However, it would cause an AttributeError saying that "AttributeError: 'Message' object has no attribute 'to_dict'". I also tried using the PlainReporter and ColorReporter and it seems that the message id W0012 was not being reported even though it was being reported after switching to the HTMLReporter.
  • When using the ColorReporter or PlainReporter, the code snippet shown is not necessarily the line in which the error occurs in the config file. It always seems to show the 3 lines of the config file regardless of it's actual location in the config file.
    image
  • With the latest changes in commit 48209d1, the reporter will print the messages in the most recent config file parsed. In theory, there is a corner case where the default PythonTA config file may have an error, as well as the provided config, but these reporters will not show the error in the default config. In practice, I believe this shouldn't be a big deal because the most recent one checked should be the only one that could possibly contain an error (since I don't believe we will ship the default Python config file with errors).

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 Jun 27, 2023

Pull Request Test Coverage Report for Build 5492948948

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

Totals Coverage Status
Change from base Build 5470955468: 0.05%
Covered Lines: 1791
Relevant Lines: 1991

💛 - 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. In addition to the comment I left, please provide information about the errors/problems you encountered when try to test this behaviour with the other reporter types.

python_ta/reporters/core.py Outdated Show resolved Hide resolved
@sushimon sushimon force-pushed the reporting-errors-in-parsing-config branch from 5385a28 to 250d7b0 Compare June 28, 2023 01:05
@david-yz-liu
Copy link
Contributor

@sushimon okay great, you should investigate the JSONReporter error further. to_dict is being defined in NewMessage, which is supposed to replace Message. It seems like there's a case where this replacement isn't happening.

@david-yz-liu
Copy link
Contributor

@sushimon oh and for the second issue, yeah you can look into that as well. Note that check_all returns the reporter object; you can use this to determine whether the message was stored properly, and thus whether the problem is in recording the message or displaying it.

linter.set_reporter(current_reporter)

if not is_any_file_checked:
current_reporter = linter.reporter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if updating current_reporter is okay. My reasoning was that since we set up this linter using the same config file as the one on line 117, this is the same reporter, except it is guaranteed that the instances of Message from the config file errors have been converted to NewMessage instances.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sushimon yes this is great!

@sushimon sushimon force-pushed the reporting-errors-in-parsing-config branch from 448ab5b to 1b643f2 Compare July 8, 2023 05:49
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 great work!

linter.set_reporter(current_reporter)

if not is_any_file_checked:
current_reporter = linter.reporter
Copy link
Contributor

Choose a reason for hiding this comment

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

@sushimon yes this is great!

@david-yz-liu david-yz-liu merged commit 43240cf into pyta-uoft:master Jul 10, 2023
@sushimon sushimon deleted the reporting-errors-in-parsing-config branch July 11, 2023 23:15
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