https://github.com/jackdewinter/pymarkdown/issues/1169 #1194
Conversation
Reviewer's Guide by SourceryThis pull request addresses several issues related to handling fenced code blocks within nested list structures and block quotes. The changes primarily focus on improving the parsing logic for these complex Markdown structures, ensuring correct indentation handling, and fixing edge cases that were previously causing problems. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jackdewinter - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider breaking down the
__detect_list_already_added_tomethod into smaller, more focused functions to improve readability and maintainability. - Adding comments to explain the logic in the
__detect_list_already_added_tomethod would be helpful for future maintenance.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -209,6 +212,53 @@ def __handle_leaf_start_adjust( | |||
|
|
|||
| # pylint: enable=too-many-arguments | |||
|
|
|||
| @staticmethod | |||
| def __detect_list_already_added_to(parser_state) -> bool: | |||
There was a problem hiding this comment.
suggestion: Consider refactoring the __detect_list_already_added_to method for clarity and simplicity.
The method is quite complex and could benefit from breaking down into smaller, more focused functions. Also, consider using more descriptive variable names instead of 'aa_line', 'aa_column', etc.
| # while ( | ||
| # stack_index > 0 | ||
| # and not parser_state.token_stack[stack_index].is_list | ||
| # and not parser_state.token_stack[stack_index].is_block_quote | ||
| # ): |
There was a problem hiding this comment.
suggestion: Remove commented-out code block if no longer needed.
stack_index = len(parser_state.token_stack) - 1
assert parser_state.token_stack[stack_index].is_list or parser_state.token_stack[stack_index].is_block_quote
bb_line = parser_state.token_stack[stack_index].matching_markdown_token.line_number
| @@ -434,6 +434,7 @@ def __process_fenced_start( | |||
| old_top_of_stack, | |||
| new_tokens, | |||
| alt_removed_chars_at_start=removed_char_length, | |||
| original_line=original_line | |||
There was a problem hiding this comment.
suggestion: Document the purpose of the new 'original_line' parameter in __handle_leaf_start method.
original_line=original_line, # Original line content for reference
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| @pytest.mark.skip | ||
| def test_extra_046f0(): |
There was a problem hiding this comment.
question (testing): Test case test_extra_046f0 has been re-enabled
The test case was previously skipped due to an issue. Now that it's enabled, please verify that the issue has been resolved and the test passes as expected.
| pluginRuleTest( # test_extra_046u0 test_extra_046u1 | ||
| "bad_fenced_block_in_list_in_block_quote_in_block_quote_with_previous_list", |
There was a problem hiding this comment.
suggestion (testing): Reference to GitHub issue removed from test comment
The GitHub issue reference has been removed. Please ensure that the issue has been resolved and add a brief comment explaining the purpose of this test case.
| pluginRuleTest( # test_extra_046u0 test_extra_046u1 | |
| "bad_fenced_block_in_list_in_block_quote_in_block_quote_with_previous_list", | |
| pluginRuleTest( | |
| "bad_fenced_block_in_list_in_block_quote_in_block_quote_with_previous_list", |
| - [Issue 1176](https://github.com/jackdewinter/pymarkdown/issues/1176) | ||
| - fixed issue with extra leading space item being added |
There was a problem hiding this comment.
suggestion (documentation): Consider consistent formatting for all changelog entries
The last item has an indented sub-point, which is inconsistent with the other entries. For better readability and consistency, you might want to either add sub-points to all entries or incorporate this information into the main bullet point.
| - [Issue 1176](https://github.com/jackdewinter/pymarkdown/issues/1176) | |
| - fixed issue with extra leading space item being added | |
| [Issue 1176](https://github.com/jackdewinter/pymarkdown/issues/1176): Fixed issue with extra leading space item being added |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1194 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 190 190
Lines 20668 20715 +47
Branches 2637 2643 +6
=======================================
+ Hits 20653 20700 +47
Misses 10 10
Partials 5 5 ☔ View full report in Codecov by Sentry. |
#1169
Summary by Sourcery
Fix issues with extra leading space items in list parsing, enhance list detection logic, update tests to reflect these fixes, and document the changes in the changelog.
Bug Fixes:
Enhancements:
__detect_list_already_added_toinleaf_block_helper.pyto improve the handling of list detection in the parser state.Documentation:
Tests:
test_markdown_extra.pyto reflect the fixed issues and ensure proper handling of list and block quote structures.test_extra_046f0ato verify the correct parsing of nested lists and code blocks within block quotes.