https://github.com/jackdewinter/pymarkdown/issues/1233 #1241
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request addresses multiple issues related to handling nested block quotes, lists, and fenced code blocks in Markdown parsing. The changes primarily affect the test suite, adding numerous new test cases to cover various edge cases and scenarios. The main implementation changes are in the container_helper.py file, modifying how nested block quotes and lists are processed. Class diagram for test cases in nested_threeclassDiagram
class TestMarkdownNestedThreeBlockUnorderedBlock {
+test_nested_three_block_unordered_block_fenced_empty_with_thematics()
+test_nested_three_block_unordered_block_fenced_empty_with_blanks_with_thematics()
+test_nested_three_block_unordered_block_fenced_with_thematics()
+test_nested_three_block_unordered_block_fenced_with_blanks_with_thematics()
+test_nested_three_block_unordered_block_fenced_with_text_before_with_thematics()
+test_nested_three_block_unordered_block_fenced_with_blanks_with_text_before_with_thematics()
+test_nested_three_block_unordered_block_fenced_with_text_after_with_thematics()
+test_nested_three_block_unordered_block_fenced_with_blanks_with_text_after_with_thematics()
+test_nested_three_block_unordered_block_fenced_multiline_with_thematics()
+test_nested_three_block_unordered_block_thematics_around_text_lines()
+test_nested_three_block_unordered_block_thematics_around_blanks_around_text_lines()
+test_nested_three_block_unordered_block_thematics_around_text_lines_with_text_after()
+test_nested_three_block_unordered_nl_block_drop_block_thematics_around_fenced_code_block()
+test_nested_three_block_unordered_nl_block_drop_block_thematics_around_blanks_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_list__li_drop_list_thematics_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_list__li_drop_list_thematics_around_blanks_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_single_line_drop_block_thematics_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_drop_block_thematics_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_drop_block_thematics_around_blanks_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_drop_block_with_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_drop_block_with_blanks_around_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_with_extra_single_indent_drop_block_with_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_with_extra_double_indent_drop_block_with_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_with_extra_triple_indent_drop_block_with_fenced_code_block()
+test_nested_three_block_unordered_block_extra_block_drop_block_extra_block_drop_block_fenced()
+test_nested_three_block_unordered_block_extra_block_drop_block_extra_block_drop_block_blanks_around_fenced()
+test_nested_three_block_unordered_nl_block_drop_block_with_fenced()
+test_nested_three_block_unordered_nl_block_drop_block_with_blanks_around_fenced()
+test_nested_three_block_unordered_block_thematics_around_text_and_fenced()
+test_nested_three_block_unordered_block_thematics_around_text_and_blanks_around_fenced()
+test_nested_three_block_unordered_block_thematic_drop_block_and_list_with_text()
+test_nested_three_block_unordered_block_thematic_nl_text_drop_block_and_list_with_text()
+test_nested_three_block_unordered_block_thematic_nl_fenced_with_drop_block_li()
+test_nested_three_block_unordered_block_fenced_with_drop_right_after()
+test_nested_three_block_unordered_block_empty_fenced_with_drop_right_after()
+test_nested_three_block_unordered_block_thematics_around_blanks_around_text_drop_block_li_li_extra_block_drop_block_li()
+test_nested_three_block_unordered_block_thematics_around_blank_drop_block_li_text()
+test_nested_three_block_unordered_block_thematics_around_blank_drop_block_li_extra_block_text()
+test_nested_three_block_unordered_nl_block_drop_block_headings_around_fenced()
+test_nested_three_block_unordered_nl_block_drop_block_blank_thematics_around_fenced()
+test_nested_three_block_unordered_extra_block_with_no_space_after_drop_block_thematics_around_fenced()
+test_nested_three_block_unordered_extra_block_drop_block_thematic_extra_block_drop_block_thematics_around_fenced()
+test_nested_three_block_unordered_extra_list_drop_list_thematics_around_fenced()
}
class TestMarkdownNestedThreeOrderedBlockUnordered {
+test_nested_three_ordered_block_unordered_nl_extra_block_drop_block_fenced()
+test_nested_three_ordered_block_unordered_nl_extra_block_drop_block_blanks_around_fenced()
+test_nested_three_ordered_block_unordered_thematics_around_fenced()
+test_nested_three_ordered_block_unordered_extra_block_drop_block_with_headings_around_fenced()
+test_nested_three_ordered_block_unordered_extra_list_li_drop_list_with_fenced()
+test_nested_three_ordered_block_unordered_extra_list_li_drop_list_with_blanks_around_fenced()
+test_nested_three_ordered_block_unordered_extra_list_li_drop_list_with_thematics_around_fenced()
+test_nested_three_ordered_block_unordered_extra_list_li_drop_list_with_thematics_around_blanks_around_fenced()
+test_nested_three_ordered_block_unordered_li_with_empty_prefix_extra_block_drop_block_headings_around_text()
+test_nested_three_ordered_block_unordered_extra_block_drop_block_headings_around_text()
+test_nested_three_ordered_block_unordered_thematics_around_headings_around_text()
+test_nested_three_ordered_block_unordered_extra_block_drop_block_thematics_around_text()
+test_nested_three_ordered_block_unordered_extra_block_drop_block_html_around_text()
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -352,6 +377,17 @@ def __abcd( | |||
| container_token_indices: List[int], | |||
| ) -> Optional[str]: | |||
| prefix = None | |||
|
|
|||
| if ( | |||
There was a problem hiding this comment.
suggestion: Consider a more general approach for handling edge cases
The new condition added to the __abcd method seems very specific. Consider refactoring this part to handle similar cases more generally, which could improve maintainability and reduce the need for future specific conditions.
if self._is_edge_case(container_stack, removed_tokens):
Comment on lines
+344
to
+353
| @staticmethod | ||
| def __abcd_inner( | ||
| removed_tokens: List[MarkdownToken], removed_token_indices: List[int] | ||
| ) -> bool: | ||
| do_check = True | ||
| if removed_tokens[0].is_list_start: | ||
| current_leading_spaces = cast( | ||
| ListStartMarkdownToken, removed_tokens[0] | ||
| ).leading_spaces | ||
| else: |
There was a problem hiding this comment.
suggestion (performance): Evaluate performance impact of method extraction
The extraction of __abcd_inner method improves readability, but ensure it doesn't introduce performance penalties if called frequently. Consider inlining if profiling shows significant overhead.
test/api/test_api_scan.py
Outdated
| @@ -184,7 +184,6 @@ def test_api_scan_for_non_markdown_file_with_alternate_extensions(): | |||
|
|
|||
|
|
|||
| @pytest.mark.timeout(30) | |||
| @pytest.mark.skip | |||
There was a problem hiding this comment.
question (testing): Removed skip mark from test_api_scan_recursive_for_directory
The skip mark has been removed from this test. Make sure that the test is now passing and that it doesn't significantly increase the overall test execution time.
Comment on lines
+172
to
177
| ( | ||
| pytest.param(i, marks=pytest.mark.skip) | ||
| if i.mark_fix_as_skipped or i.mark_scan_as_skipped | ||
| else i | ||
| ) | ||
| for i in scanTests |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for the new condition
The condition for skipping tests has been updated to include 'i.mark_scan_as_skipped'. It would be beneficial to add a test case that verifies this new condition.
@pytest.mark.parametrize(
"test_case",
[
(
pytest.param(i, marks=pytest.mark.skip)
if i.mark_fix_as_skipped or i.mark_scan_as_skipped
else i
)
for i in scanTests
],
)
def test_skip_condition(test_case):
assert not (test_case.mark_fix_as_skipped or test_case.mark_scan_as_skipped)
test/test_markdown_extra.py
Outdated
| @@ -6049,515 +6002,363 @@ def test_extra_043a(): | |||
|
|
|||
|
|
|||
| @pytest.mark.gfm | |||
| def test_extra_044x(): | |||
| def test_extra_044d(): | |||
There was a problem hiding this comment.
suggestion (testing): Addition of new test case test_extra_044d
This new test case appears to be testing fenced code blocks within nested blockquotes. Ensure it covers all relevant edge cases for this scenario.
def test_extra_044d():
"""
Test fenced code blocks within nested blockquotes.
"""
# Arrange
source_markdown = """> > inner block
> > inner block
> This is text and no blank line.
> ```block
> A code block
> ```
> This is a blank line and some text.
"""
expected_gfm = """<blockquote>
<blockquote>
<p>inner block
inner block
This is text and no blank line.</p>
</blockquote>
<pre><code class="language-block">A code block
</code></pre>
<p>This is a blank line and some text.</p>
</blockquote>"""
# Act & Assert
act_and_assert(source_markdown, expected_gfm, expected_tokens)
test/test_markdown_extra.py
Outdated
| </blockquote>""" | ||
|
|
||
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens) | ||
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_044b(): | ||
| def test_extra_044jeb(): |
There was a problem hiding this comment.
suggestion (testing): Addition of new test case test_extra_044jeb
This test case involves complex nesting of lists and blockquotes. Ensure that it adequately tests the edge cases for such structures.
"[end-ulist:::True]",
"[end-ulist:::True]",
"[end-block-quote:::True]",
"[tbreak(4,1):-::-----]",
"[fcode-block(5,1):`:3:block:::::]",
"[text(6,1):A code block:]",
"[end-fcode-block:::3:False]",
"[tbreak(8,1):-::-----]",
"[para(9,1):]",
"[text(9,1):another list:]",
test/test_markdown_extra.py
Outdated
Comment on lines
6164
to
6165
| @pytest.mark.skip | ||
| def test_extra_044jec(): |
There was a problem hiding this comment.
suggestion (testing): New skipped test case test_extra_044jec
This test is marked to be skipped. Consider adding a comment explaining why it's skipped and when it should be unskipped.
@pytest.mark.skip(reason="TBD: Explain why this test is skipped and when it should be unskipped")
def test_extra_044jec():
newdocs/src/changelog.md
Outdated
| @@ -15,7 +15,9 @@ | |||
| <!--- pyml disable-next-line no-duplicate-heading--> | |||
| ### Changed | |||
|
|
|||
| - None | |||
| - [Issue 1231)](https://github.com/jackdewinter/pymarkdown/issues/1231) | |||
There was a problem hiding this comment.
issue (documentation): Fix typo in issue number link
There's an extra closing parenthesis after the issue number. It should be '[Issue 1231]' instead of '[Issue 1231)]'.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1241 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 191 191
Lines 21037 21081 +44
Branches 2683 2692 +9
=========================================
+ Hits 21037 21081 +44 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #1233
closes #1234
closes #1235
Summary by Sourcery
Fix block quote handling in container reduction and add comprehensive tests for nested structures with fenced code blocks.
Bug Fixes:
Tests: