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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 15 commits into from
Jul 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
193521f
Updated number of error occurrences reported and fixed bug in HTML re…
sarahsonder Jun 14, 2023
e0a447a
Updated CHANGELOG and fixed typo in docs
sarahsonder Jun 14, 2023
b03c2a0
Merge branch 'master' into update-number-of-error-occurrences-reported
sarahsonder Jun 24, 2023
f9d3f45
Updated number of error occurrences reported and fixed bug in HTML re…
sarahsonder Jun 14, 2023
9be0981
Updated CHANGELOG and fixed typo in docs
sarahsonder Jun 14, 2023
da4917a
Changed JSON Reporter behaviour and added tests
sarahsonder Jul 1, 2023
39f4203
Resolved conflicts between branches
sarahsonder Jul 1, 2023
c1885ac
Clarified docstrings and fixed typos
sarahsonder Jul 1, 2023
6b48061
Changed list and dict to use typing List and Dict
sarahsonder Jul 3, 2023
43301b7
Optimized JSON Reporter and Updated Changelog
sarahsonder Jul 3, 2023
013b765
Merge branch 'master' of https://github.com/pyta-uoft/pyta into updat…
sarahsonder Jul 3, 2023
f89ad9d
Merge branch 'master' into update-number-of-error-occurrences-reported
sarahsonder Jul 3, 2023
6f91055
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 3, 2023
b1e19de
Optimizing JSON Reporter 2.0
sarahsonder Jul 3, 2023
5be4fcc
Merge branch 'update-number-of-error-occurrences-reported' of https:/…
sarahsonder Jul 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
- For the message display of E9989 `pep8-errors`, all "blank line" messages are now custom-rendered, i.e., blank lines
are now highlighted instead of function signatures and instruction strings are added for required blank lines that
are missing.
- The default value of `pyta-number-of-messages` is now 0. This automatically displays all occurrences of the same error.

### Bug Fixes

- Fixed bug where running `python3 -m python_ta --generate-config` yields a `FileNotFoundError`.
- Fixed bug where the HTML reporter would display all error occurrences of the same type despite stating that only a limited number was being shown.
david-yz-liu marked this conversation as resolved.
Show resolved Hide resolved

### New checkers

Expand Down
2 changes: 1 addition & 1 deletion docs/usage/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Configuration

```{note}
This page is current under construction!
This page is currently under construction!
```

## Providing Your Own Configuration Settings
Expand Down
2 changes: 1 addition & 1 deletion python_ta/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ def reset_linter(
(
"pyta-number-of-messages",
{
"default": 5,
"default": 0, # If the value is 0, all messages are displayed.
"type": "int",
"metavar": "<number_messages>",
"help": "Display a certain number of messages to the user, without overwhelming them.",
david-yz-liu marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
4 changes: 2 additions & 2 deletions python_ta/config/.pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

# Make sure to register custom options tuple first in `python_ta/__init__.py`
# ===========================================================
# Default max amount of messages for reporter to display.
pyta-number-of-messages = 5
# Default max amount of messages for reporter to display. If the value is 0, all messages are displayed.
pyta-number-of-messages = 0

# (DEPRECATED: Use output-format option below.) Set the [REPORTS] output-format option instead.
# pyta-reporter = HTMLReporter
Expand Down
30 changes: 29 additions & 1 deletion python_ta/reporters/json_reporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,36 @@ def display_messages(self, layout: BaseLayout) -> None:
output.append(
{
"filename": k,
"msgs": [msg.to_dict() for msg in msgs],
"msgs": self._output_messages(msgs),
}
)

self.writeln(json.dumps(output, indent=4))

def _output_messages(self, msgs: List[NewMessage]) -> List[Dict]:
"""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.)


unique_msg_names = []
[
unique_msg_names.append(msg_id)
for msg_id in all_msg_names
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!

num_messages = 0
for msg in msgs:
if max_messages == 0 or (
msg.message.msg_id == msg_id and num_messages < max_messages
):
message_dict = msg.to_dict()
message_dict["number_of_occurrences"] = all_msg_names.count(msg.message.msg_id)

if message_dict not in output_lst:
output_lst.append(message_dict)
num_messages += 1

return output_lst
8 changes: 6 additions & 2 deletions python_ta/reporters/plain_reporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,16 @@ def _colour_messages_by_type(self, messages: Dict[str, List[NewMessage]]) -> str
result += self._colourify("bold", msg_id)
result += self._colourify("bold", " ({}) ".format(messages[msg_id][0].symbol))
result += "Number of occurrences: {}.".format(len(messages[msg_id]))
if max_messages != float("inf") and max_messages < len(messages[msg_id]):
if (
max_messages != 0
and max_messages != float("inf")
and max_messages < len(messages[msg_id])
):
result += " (First {} shown).".format(max_messages)
result += self._BREAK

for i, msg in enumerate(messages[msg_id]):
if i == max_messages:
if max_messages != 0 and i == max_messages:
break

# Use only explanation, without redundant accessory information
Expand Down
93 changes: 48 additions & 45 deletions python_ta/reporters/templates/template.html.jinja
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
{% endif %}
</span>
<span class="shown">
{% if limit < num_occurrences %}
{% if 0 < limit < num_occurrences %}
(First {{ limit }} shown).
{% endif %}
</span>
Expand All @@ -141,27 +141,29 @@
</div>
<div class="message-container collapsible expanded">
{% for indiv in occurrences %}
<div class="message">
<p class="message-name">
[Line {{ indiv.line }}] {{ indiv.msg }}
<span class="slider">
<button style="float: right;">
<svg class="collapse-trigger" viewBox="0 1 10 10" width="60" height="20">
<g transform="scale(0.13 0.13)">
<path d="M 10 30 l 40 40 l 40 -40" stroke="#777777" stroke-width="15px" fill="transparent"/>
</g>
</svg>
</button>
</span>
</p>
{% if indiv.snippet != '' %}
<div class="collapsible expanded message-snippet-container">
<pre class="message-snippet">{{ indiv.snippet }}</pre>
</div>
{% else %}
<span class="empty-placeholder">{{ reporter.no_snippet }}</span>
{% endif %}
</div>
{% if limit == 0 or loop.index0 < limit %}
<div class="message">
<p class="message-name">
[Line {{ indiv.line }}] {{ indiv.msg }}
<span class="slider">
<button style="float: right;">
<svg class="collapse-trigger" viewBox="0 1 10 10" width="60" height="20">
<g transform="scale(0.13 0.13)">
<path d="M 10 30 l 40 40 l 40 -40" stroke="#777777" stroke-width="15px" fill="transparent"/>
</g>
</svg>
</button>
</span>
</p>
{% if indiv.snippet != '' %}
<div class="collapsible expanded message-snippet-container">
<pre class="message-snippet">{{ indiv.snippet }}</pre>
</div>
{% else %}
<span class="empty-placeholder">{{ reporter.no_snippet }}</span>
{% endif %}
</div>
{% endif %}
{% endfor %}
</div>
</div>
Expand Down Expand Up @@ -201,7 +203,7 @@
{% endif %}
</span>
<span class="shown">
{% if limit < num_occurrences %}
{% if 0 < limit < num_occurrences %}
(First {{ limit }} shown).
{% endif %}
</span>
Expand All @@ -215,30 +217,31 @@
</button>
</span>
</div>

<div class="message-container collapsible expanded">
{% for indiv in occurrences %}
<div class="message">
<p class="message-name">
[Line {{ indiv.line }}] {{ indiv.msg }}
<span class="slider">
<button style="float: right;">
<svg class="collapse-trigger" viewBox="0 1 10 10" width="60" height="20">
<g transform="scale(0.13 0.13)">
<path d="M 10 30 l 40 40 l 40 -40" stroke="#777777" stroke-width="15px" fill="transparent"/>
</g>
</svg>
</button>
</span>
</p>
<div class="collapsible expanded message-snippet-container">
{% if indiv.snippet != '' %}
<pre class="message-snippet">{{ indiv.snippet }}</pre>
{% else %}
<span class="empty-placeholder">{{ reporter.no_snippet }}</span>
{% endif %}
</div>
</div>
{% if limit == 0 or loop.index0 < limit %}
<div class="message">
<p class="message-name">
[Line {{ indiv.line }}] {{ indiv.msg }}
<span class="slider">
<button style="float: right;">
<svg class="collapse-trigger" viewBox="0 1 10 10" width="60" height="20">
<g transform="scale(0.13 0.13)">
<path d="M 10 30 l 40 40 l 40 -40" stroke="#777777" stroke-width="15px" fill="transparent"/>
</g>
</svg>
</button>
</span>
</p>
<div class="collapsible expanded message-snippet-container">
{% if indiv.snippet != '' %}
<pre class="message-snippet">{{ indiv.snippet }}</pre>
{% else %}
<span class="empty-placeholder">{{ reporter.no_snippet }}</span>
{% endif %}
</div>
</div>
{% endif %}
{% endfor %}
</div>

Expand Down
2 changes: 1 addition & 1 deletion tests/test.pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# Make sure to register custom options tuple first in `python_ta/__init__.py`
# ===========================================================
# Default max amount of messages for reporter to display.
pyta-number-of-messages = 5
pyta-number-of-messages = 0

# (DEPRECATED: Use output-format option below.) Set the [REPORTS] output-format option instead.
# pyta-reporter = HTMLReporter
Expand Down
32 changes: 32 additions & 0 deletions tests/test_config/file_fixtures/funcs_with_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
"""Python script used for testing that the correct number of error occurrences are being displayed."""
from typing import List

# The following imports are used solely to trigger errors.
import packaging
import pip
import pygments
import pylint


def sum_items(lst: List[int]) -> int:
"""..."""
s = 0
for i in range(len(lst)):
s += lst[i]
return s


def sum_items2(lst: List[int]) -> int:
"""..."""
s = 0
for i in range(0, len(lst)):
s += lst[i]
return s


def sum_items3(lst: List[int]) -> int:
"""..."""
s = 0
for i in range(0, len(lst), 1):
s += lst[i]
return s
55 changes: 55 additions & 0 deletions tests/test_config/test_num_error_occurrences.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
"""
Test suite for checking that the correct number of error occurrences are being displayed.
"""

import contextlib
import io
import os

from python_ta import check_all


def pyta_output(num_msgs: int) -> str:
"""Returns the PythonTA report as a string."""
output = io.StringIO()

curr_dir = os.path.dirname(__file__)
test_file = os.path.join(curr_dir, "file_fixtures", "funcs_with_errors.py")

with contextlib.redirect_stdout(output):
check_all(
module_name=test_file,
config={
"pyta-number-of-messages": num_msgs,
"output-format": "python_ta.reporters.JSONReporter",
},
)

return output.getvalue()


def test_default() -> None:
"""Tests that all messages are displayed when pyta-number-of-messages = 0."""
pyta_report = pyta_output(0)
expected = 12
actual = pyta_report.count("msg_id")

assert expected == actual


def test_num_msgs2() -> None:
"""Tests that only two messages per error are displayed when pyta-number-of-messages = 2."""
pyta_report = pyta_output(2)
expected = 7
actual = pyta_report.count("msg_id")

assert expected == actual


def test_num_msgs_greater() -> None:
"""Tests that all messages are displayed when pyta-number-of-messages is greater than the number of errors."""
pyta_report = pyta_output(5)
expected = 12
actual = pyta_report.count("msg_id")

assert expected == actual
2 changes: 1 addition & 1 deletion tests/test_messages_config/test.pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# Make sure to register custom options tuple first in `python_ta/__init__.py`
# ===========================================================
# Default max amount of messages for reporter to display.
pyta-number-of-messages = 5
pyta-number-of-messages = 0

# (DEPRECATED: Use output-format option below.) Set the [REPORTS] output-format option instead.
# pyta-reporter = HTMLReporter
Expand Down