Fixing node_printers.py issue caused by refactoring #1239

Zain-Mahmoud · 2025-09-30T03:16:46Z

Proposed Changes

Fixed issue where some node_printers.py renderers weren't properly displaying the error messages. This error was caused by the recent refactoring of node_printers.py

...

Screenshots of your changes (if applicable)

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)	x
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📚 Documentation update (change that only updates documentation)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
I have updated the project Changelog (this is required for all changes).
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

coveralls · 2025-09-30T03:52:07Z

Pull Request Test Coverage Report for Build 18393632544

Details

48 of 48 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.05%) to 94.798%

Totals
Change from base Build 18296870261:	0.05%
Covered Lines:	3517
Relevant Lines:	3710

💛 - Coveralls

david-yz-liu · 2025-10-02T21:47:53Z

python_ta/reporters/node_printers.py

-            yield from render_context(line - 3, line, source_lines)
-            yield from RENDERERS[error_code](msg, line, col, source_lines[line - 1])
-            yield from render_context(line + 1, line + 3, source_lines)
+            if error_code in {"E301", "E302", "E303", "E304", "E305", "E306"}:


Define a constant for this set, making sure to give it a meaningful name

david-yz-liu · 2025-10-02T21:49:28Z

CHANGELOG.md


 ### 🐛 Bug fixes

+- Fixed issue that caused some error messages to not render properly


let's be more specific here (refer to the pep8-serrors check)

david-yz-liu · 2025-10-02T22:02:30Z

python_ta/reporters/node_printers.py



-def render_pep8_errors_e306(msg, line, col, source_line=None):
+def render_pep8_errors_e306(msg, source_lines=None):


Same comment as E301

david-yz-liu · 2025-10-02T22:02:46Z

python_ta/reporters/node_printers.py



-def render_pep8_errors_e301(msg, line, col, source_line=None):
+def render_pep8_errors_e301(msg, source_lines=None):


This function doesn't need to use source_lines. Instead, it just needs to yield both the message (NEW_BLANK_LINE_MESSAGE) and the passed in source_line

david-yz-liu · 2025-10-02T22:08:50Z

python_ta/reporters/node_printers.py

-    line -= 1
+    line = msg.line - 1
    if "found 0" in msg.msg:
+        yield from render_context(line - 1, line + 1, source_lines)


Overall, the same number of lines of context (3) should be shown for all error messages. This means you'll need to update the custom renderers that use source_lines directly, so that they're consistent with generic approach (Lines 158-160).

In general, this code can be simplified to eliminate some +1/-1 adjustments and make the math more straightforward.

Set line = msg.line (the standard path on Line 156). Write all render_context calls in terms of line (not msg.line, like on Line 429). Eliminate the line -= 1 statement on Line 425 (changing variable values makes code more complex).

david-yz-liu · 2025-10-02T22:13:21Z

python_ta/reporters/node_printers.py

        line -= 1
-
-    body = source_line[msg.line - 1]
+    yield from render_context(line - 1, line + 1, source_lines)


Write all of the yield statements together

david-yz-liu · 2025-10-02T22:13:41Z

python_ta/reporters/node_printers.py

        (curr_line, slice(None, None), LineType.ERROR, " " * (indentation + 28))
        for curr_line in range(line + 1, msg.line)
    )
+    yield from render_context(msg.line, msg.line + 2, source_lines)


As above, only yield lines using the line variable, not msg.line

david-yz-liu · 2025-10-02T22:14:36Z

python_ta/reporters/node_printers.py



-def render_pep8_errors_e304(msg, line, col, source_line=None):
+def render_pep8_errors_e304(msg, source_lines=None):


I think you can just combine this function with E303. The only difference is the presence of indentation in E303, which is actually good to do for E304 as well.

david-yz-liu · 2025-10-02T22:14:54Z

python_ta/reporters/node_printers.py

+def render_pep8_errors_e305(msg, source_lines=None):
    """Render a PEP8 expected 2 blank lines after class or function definition message."""
-    line -= 1
+    line = msg.line - 1


Same comments as above

david-yz-liu · 2025-10-02T22:19:52Z

@Zain-Mahmoud oh right I forgot to mention, you'll also need to do a pull from upstream master in order to get the latest changes. There will be a small merge conflict to resolve; this should be fairly straightforward since this is related to your other PR, but I encourage you to ask your teammates for guidance if you get stuck.

…ork into fixing-node-printers

david-yz-liu

Nice work, @Zain-Mahmoud!

Zain-Mahmoud added 2 commits September 29, 2025 23:11

fixing node_printers.py issue caused by refactoring

7106500

updated CHANGELOG.md

838ed91

Zain-Mahmoud requested a review from david-yz-liu September 30, 2025 03:54

david-yz-liu reviewed Oct 2, 2025

View reviewed changes

Zain-Mahmoud added 3 commits October 9, 2025 18:43

updated structure of renderers requiring source_lines

90ea5e6

updated CHANGELOG.md

ef490e9

Merge branch 'master' into fixing-node-printers

241c352

Zain-Mahmoud requested a review from david-yz-liu October 9, 2025 23:43

Zain-Mahmoud added 2 commits October 9, 2025 20:58

updating combined function's docstring

2cc2c4d

Merge branch 'fixing-node-printers' of github.com:Zain-Mahmoud/pyta-f…

4862ad2

…ork into fixing-node-printers

david-yz-liu approved these changes Oct 10, 2025

View reviewed changes

david-yz-liu merged commit f2d8551 into pyta-uoft:master Oct 10, 2025
25 checks passed


		### 🐛 Bug fixes

		- Fixed issue that caused some error messages to not render properly



		def render_pep8_errors_e306(msg, line, col, source_line=None):
		def render_pep8_errors_e306(msg, source_lines=None):



		def render_pep8_errors_e301(msg, line, col, source_line=None):
		def render_pep8_errors_e301(msg, source_lines=None):



		def render_pep8_errors_e304(msg, line, col, source_line=None):
		def render_pep8_errors_e304(msg, source_lines=None):

Fixing node_printers.py issue caused by refactoring #1239

Fixing node_printers.py issue caused by refactoring #1239

Uh oh!

Conversation

Zain-Mahmoud commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Type of Change

Checklist

Questions and Comments

Uh oh!

coveralls commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18393632544

Details

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-yz-liu commented Oct 2, 2025

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Zain-Mahmoud commented Sep 30, 2025 •

edited

Loading

coveralls commented Sep 30, 2025 •

edited

Loading