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

Update number of error occurrences reported + Bug fix in HTML reporter #915

Conversation

sarahsonder
Copy link
Contributor

@sarahsonder sarahsonder commented Jun 14, 2023

Motivation and Context

Currently, PythonTA limits the maximum number of the times the same error can shown at 5 errors. However, students have requested for all error occurrences to be shown. Furthermore, there was a bug in the HTML reporter which displayed all error occurrences despite the reporter stating that only a limited number of occurrences were being shown.

Your Changes

Description:

  1. Changed the default value of pyta-number-of-messages to 0. This by default displays all instances of the same error.
  2. Updated the behaviour of plain_reporter so that it limits the number of error occurrences ONLY when pyta-number-of-messages is not 0.
  3. Added limiting behaviour to html_reporter so that its behaviour mirrors that of plain_reporter (previously html_reporter was not limiting the number of errors being shown).
  4. Updated the conditions for "First shown" to be displayed only when pyta-number-of-messages is not 0 and the number of error messages exceeds pyta-number-of-messages.
  5. Fixed typo in the Configuration page of the PythonTA documentation.

Type of change (select all that apply):

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update (change that modifies or updates documentation only)

Testing

I tested my code by running both plain_reporter and html_reporter on three cases:

  1. Setting pyta-number-of-messages = 0 to check that all error occurrences are being shown
  2. Setting pyta-number-of-messages = 2 and running PythonTA on a file that has 5 of the same errors. This was to check that PythonTA was limiting the number of messages shown.
  3. Setting pyta-number-of-messages = 6 and running PythonTA on a file that has 5 of the same errors. This was to check that PythonTA would still display all error occurrences.

Questions and Comments

I noticed that the Configuration page in the PythonTA documentation is under construction. Is there anything that I can add to the page?

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 updated the CHANGELOG.md file.

@coveralls
Copy link
Collaborator

coveralls commented Jun 14, 2023

Pull Request Test Coverage Report for Build 5449391557

  • 33 of 50 (66.0%) changed or added relevant lines in 3 files are covered.
  • 19 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.5%) to 89.909%

Changes Missing Coverage Covered Lines Changed/Added Lines %
python_ta/reporters/templates/template.html.jinja 31 48 64.58%
Files with Coverage Reduction New Missed Lines %
python_ta/reporters/templates/template.html.jinja 19 67.83%
Totals Coverage Status
Change from base Build 5449285850: 0.5%
Covered Lines: 1782
Relevant Lines: 1982

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

@sarahsonder great work! I just left a few minor comments, in addition to my earlier feedback of adding some test cases. 😄

That said, I also realized that I said to use the JSON reporter for testing, but based on the changes you made, I'm not sure whether this reporter is actually using this configuration at all? If not, it would be great if you fixed that in this PR as well.

CHANGELOG.md Outdated Show resolved Hide resolved
python_ta/__init__.py Show resolved Hide resolved
python_ta/config/.pylintrc Outdated Show resolved Hide resolved
@sarahsonder
Copy link
Contributor Author

@david-yz-liu Hi Prof. Liu! I have updated the JSON reporter behaviour to limit the number of error messages and have added tests for my changes. The tests themselves work and the JSON reporter also working when I run PyTA on other files. However, the git tests are not passing due to a type error ('type' object is not subscriptable) caused by the helper function that I wrote in the JSON reporter. Could I please get some advice on how to resolve this error? Thank you!

@sarahsonder
Copy link
Contributor Author

@david-yz-liu Issue fixed! I needed to use List and Dict instead of list and dict.

CHANGELOG.md Show resolved Hide resolved
"""Returns a list of dictionaries containing formatted error messages."""
max_messages = self.linter.config.pyta_number_of_messages
output_lst = []
all_msg_names = [msg.message.msg_id for msg in msgs]
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't necessary to define this separately from unique_msg_names. You can instead use a set comprehension to automatically remove duplicates.

(But also see my below comment for additional suggestions for this part.)

if msg_id not in unique_msg_names
]

for msg_id in unique_msg_names:
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is okay but is less efficient than it could be. Instead try this approach:

  • loop over just msgs
  • use a dict accumulator mapping error msg_id to the count of that message type "seen so far" in the loop. Note that you can initialize the accumulator using all of the unique keys (see above comprehension) with initial corresponding value 0
  • then inside the loop use the accumulator to decide whether to append a message to output_lst or not
  • after that loop, use a second loop over output_lst that sets the number_of_occurrences for each output message---we don't need to bother with updating this for messages that aren't included in the output!

@david-yz-liu
Copy link
Contributor

@sarahsonder I left a review but accidentally selected "Approve" instead of "Comment". So just to be clear, please address the review comments I left and then re-request a review. Thanks! 😄

@david-yz-liu david-yz-liu merged commit a062b0a into pyta-uoft:master Jul 6, 2023
5 checks passed
@sarahsonder sarahsonder deleted the update-number-of-error-occurrences-reported branch July 6, 2023 14:41
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