Issue 818 #1185
Issue 818 #1185
Conversation
Reviewer's Guide by SourceryThis pull request implements fixes and improvements for handling fenced code blocks within various nested container structures like block quotes and lists. It addresses several issues related to proper indentation, blank line handling, and token adjustments for fenced code blocks in complex Markdown structures. 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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 11 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.
| ) | ||
| POGGER.debug(f"pre>:{pre_container_text}:<") | ||
| POGGER.debug(f"adj>:{adjusted_text}:<") | ||
| transformed_data = pre_container_text + adjusted_text | ||
| POGGER.debug(f"trn>:{transformed_data}:<") | ||
| return transformed_data | ||
|
|
||
| # pylint: disable=too-many-arguments | ||
| # pylint: disable=too-many-arguments, too-many-locals | ||
| @staticmethod | ||
| def __apply_line_transformation( |
There was a problem hiding this comment.
suggestion: Refactor __apply_line_transformation to reduce parameter count
The __apply_line_transformation method has gained several new parameters. Consider using a parameter object or breaking the function into smaller parts to reduce complexity.
def __apply_line_transformation(self, transformation_data: LineTransformationData):
return self.__apply_line_transformation_impl(**transformation_data.__dict__)
@staticmethod
def __apply_line_transformation_impl(
| @@ -490,7 +490,13 @@ def __list_in_process_update_containers( | |||
| ListStartMarkdownToken, | |||
| parser_state.token_stack[ind].matching_markdown_token, | |||
| ) | |||
| POGGER.debug( | |||
There was a problem hiding this comment.
suggestion: Consider combining repeated debug logs into a single, more informative statement
Instead of logging before and after modifying list_token, you could log once with both the before and after states, reducing code duplication and providing more context in a single log entry.
POGGER.debug(
"__adjust_for_list_container_after_block_quote_special_special>>block_token>>before_after",
f"Before: {xx_block_quote_token}",
f"After: {xx_block_quote_token_after_modification}"
)
| @staticmethod | ||
| def is_fenced_code_block( | ||
| parser_state: ParserState, |
There was a problem hiding this comment.
suggestion: Update the method's docstring to reflect new parameters
The is_fenced_code_block method has new parameters, but the docstring hasn't been updated. Please add descriptions for parser_state, original_line, and index_indent to maintain clear documentation.
| </blockquote>""" | ||
|
|
||
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens) | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens, show_debug=True) |
There was a problem hiding this comment.
suggestion (testing): Debug output enabled for specific test cases
Enabling debug output for test cases 229ha and 229ja could help with troubleshooting. Consider removing or commenting out these debug flags before merging if they're not needed for regular test runs.
| @@ -1741,7 +1741,7 @@ def test_nested_three_block_max_ordered_max_block_max_no_bq2_with_li(): | |||
|
|
|||
|
|
|||
| @pytest.mark.gfm | |||
There was a problem hiding this comment.
suggestion (testing): Test renamed to indicate it's being worked on
The 'x' suffix suggests this test is being modified or investigated. Consider adding a comment explaining why this test is marked, or remove the suffix if the changes are complete.
@pytest.mark.gfm
def test_nested_three_block_max_ordered_max_block_max_empty_no_bq2():
"""
Verify that a nesting of block quote, ordered list, block quote, with
the maximum number of spaces allowed, and no text on the first line, works properly,
</blockquote>"""
# Act & Assert
act_and_assert(source_markdown, expected_gfm, expected_tokens, show_debug=True)
@pytest.mark.gfm
| @@ -7705,7 +7708,7 @@ def test_extra_044jec(): | |||
|
|
|||
|
|
|||
| @pytest.mark.gfm | |||
| def test_extra_044k(): | |||
| def test_extra_044kx(): | |||
There was a problem hiding this comment.
question (testing): Test name changed without explanation
The test name has been modified. It would be helpful to understand the reason for this change. Does it reflect a change in the test's purpose or scope?
test/test_markdown_extra.py
Outdated
| <ol> | ||
| <li>another list</li> | ||
| </ol>""" | ||
|
|
||
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens) | ||
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_046w1(): | ||
| """ | ||
| TBD | ||
| bad_fenced_block_surrounded_by_list | ||
| """ | ||
|
|
||
| # Arrange | ||
| source_markdown = """+ list | ||
|
|
||
| ```block | ||
| A code block | ||
| ``` | ||
|
|
||
| 1. another list | ||
| """ | ||
| expected_tokens = [ | ||
| "[ulist(1,1):+::2::]", | ||
| "[para(1,3):]", | ||
| "[text(1,3):list:]", | ||
| "[end-para:::True]", | ||
| "[BLANK(2,1):]", | ||
| "[end-ulist:::True]", | ||
| "[fcode-block(3,1):`:3:block:::::]", | ||
| "[text(4,1):A code block:]", | ||
| "[end-fcode-block:::3:False]", | ||
| "[BLANK(6,1):]", | ||
| "[olist(7,1):.:1:3::]", | ||
| "[para(7,4):]", | ||
| "[text(7,4):another list:]", | ||
| "[end-para:::True]", | ||
| "[BLANK(8,1):]", | ||
| "[end-olist:::True]", | ||
| ] | ||
| expected_gfm = """<ul> | ||
| <li>list</li> | ||
| </ul> | ||
| <pre><code class="language-block">A code block | ||
| </code></pre> | ||
| <ol> | ||
| <li>another list</li> | ||
| </ol>""" | ||
|
|
||
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens) | ||
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_046x0(): | ||
| """ | ||
| TBD | ||
| bad_fenced_block_surrounded_by_block_quote | ||
| """ | ||
|
|
||
| # Arrange | ||
| source_markdown = """> block quote | ||
| ```block | ||
| A code block | ||
| ``` | ||
| > block quote | ||
| """ | ||
| expected_tokens = [ | ||
| "[block-quote(1,1)::> ]", | ||
| "[para(1,3):]", | ||
| "[text(1,3):block quote:]", | ||
| "[end-para:::True]", | ||
| "[end-block-quote:::True]", | ||
| "[fcode-block(2,1):`:3:block:::::]", | ||
| "[text(3,1):A code block:]", | ||
| "[end-fcode-block:::3:False]", | ||
| "[block-quote(5,1)::> \n]", | ||
| "[para(5,3):]", | ||
| "[text(5,3):block quote:]", | ||
| "[end-para:::True]", | ||
| "[end-block-quote:::True]", | ||
| "[BLANK(6,1):]", | ||
| ] | ||
| expected_gfm = """<blockquote> | ||
| <p>block quote</p> | ||
| </blockquote> | ||
| <pre><code class="language-block">A code block | ||
| </code></pre> | ||
| <blockquote> | ||
| <p>block quote</p> | ||
| </blockquote>""" | ||
|
|
||
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens) | ||
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_046x1(): |
There was a problem hiding this comment.
suggestion (testing): Test description is incomplete
Similar to the previous test, the description 'TBD' should be replaced with a clear explanation of the test's purpose.
def test_extra_046x1():
"""
Test fenced code block surrounded by block quotes with blank lines.
"""
# Arrange
source_markdown = """> block quote
```block
A code block
block quote
"""
expected_tokens = [
"[block-quote(1,1)::> \n]",
"[para(1,3):]",
"[text(1,3):block quote:]",
"[end-para:::True]",
"[BLANK(2,1):]",
"[end-block-quote:::True]",
"[fcode-block(3,1):`:3:block:::::]",
"[text(4,1):A code block:]",
"[end-fcode-block:::3:False]",
"[BLANK(6,1):]",
"[block-quote(7,1)::> \n]",
"[para(7,3):]",
"[text(7,3):block quote:]",
"[end-para:::True]",
"[end-block-quote:::True]",
"[BLANK(8,1):]",
]
| def test_extra_046x0(): | ||
| """ | ||
| TBD | ||
| bad_fenced_block_surrounded_by_block_quote | ||
| """ | ||
|
|
||
| # Arrange | ||
| source_markdown = """> block quote | ||
| ```block | ||
| A code block | ||
| ``` | ||
| > block quote | ||
| """ | ||
| expected_tokens = [ | ||
| "[block-quote(1,1)::> ]", | ||
| "[para(1,3):]", | ||
| "[text(1,3):block quote:]", | ||
| "[end-para:::True]", | ||
| "[end-block-quote:::True]", | ||
| "[fcode-block(2,1):`:3:block:::::]", | ||
| "[text(3,1):A code block:]", | ||
| "[end-fcode-block:::3:False]", | ||
| "[block-quote(5,1)::> \n]", | ||
| "[para(5,3):]", | ||
| "[text(5,3):block quote:]", | ||
| "[end-para:::True]", | ||
| "[end-block-quote:::True]", | ||
| "[BLANK(6,1):]", | ||
| ] | ||
| expected_gfm = """<blockquote> | ||
| <p>block quote</p> | ||
| </blockquote> | ||
| <pre><code class="language-block">A code block | ||
| </code></pre> | ||
| <blockquote> | ||
| <p>block quote</p> | ||
| </blockquote>""" | ||
|
|
||
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens) | ||
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_046x1(): |
There was a problem hiding this comment.
suggestion (testing): Tests appear to be very similar
These two tests seem to be checking very similar scenarios with only slight differences. Consider using parameterized tests to reduce code duplication and make it easier to add more similar test cases in the future.
@pytest.mark.parametrize("test_id, block_quote_content, code_block_content", [
("046x0", "block quote", "A code block"),
("046x1", "block quote\nwith two lines", "A code block\nwith two lines"),
])
def test_extra_046x(test_id, block_quote_content, code_block_content):
source_markdown = f"> {block_quote_content}\n```block\n{code_block_content}\n```\n> {block_quote_content}"
# ... rest of the test implementation
newdocs/src/changelog.md
Outdated
| of those fixes dealt with the parser, with the bulk of the issues | ||
| dealing with transitioning from Markdown to our internal token format | ||
| and back to Markdown again. | ||
|
|
||
| Why is this important? When a user asks the PyMarkdown linter to fix | ||
| any issues that it can, our team wants to have the utmost confidence | ||
| that PyMarkdown is producing the correct fix. Therefore, we tokenize | ||
| the Markdown and base our rules off tokens that we know are correct. | ||
| The only way to validate that we have the correct tokens is to take | ||
| those tokens and recreate the Markdown. If we cannot produce the |
There was a problem hiding this comment.
issue (documentation): Typo: 'if' should be 'is'
In the sentence 'the Markdown that we produce if off by a couple of whitespace characters', 'if' should be changed to 'is'.
| # f" pre_previous_token->{ParserHelper.make_value_visible(pre_previous_token)}" | ||
| # ) | ||
| if pre_previous_token.is_block_quote_start: | ||
| # sourcery skip: move-assign |
There was a problem hiding this comment.
issue (code-quality): Unused skip comment (unused-skip-comment)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1185 +/- ##
==========================================
- Coverage 99.98% 99.92% -0.06%
==========================================
Files 190 190
Lines 20228 20668 +440
Branches 2564 2637 +73
==========================================
+ Hits 20225 20653 +428
- Misses 2 10 +8
- Partials 1 5 +4 ☔ View full report in Codecov by Sentry. |
#818
More tests and such for expanding the Md031 fixes.
Summary by Sourcery
Expand test coverage for handling of fenced code blocks in nested structures, ensuring proper blank line handling and integration with thematic breaks.
Tests: