https://github.com/jackdewinter/pymarkdown/issues/1130 #1131
Conversation
Reviewer's Guide by SourceryThis pull request addresses issue #1130 by adding new test cases and making adjustments to the handling of block quotes and lists in the 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: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 9 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.
| found_bq_stack_token = cast( | ||
| BlockQuoteStackToken, parser_state.token_stack[stack_index] | ||
| ) | ||
| is_not_blank_line = bool(line_to_parse.strip(Constants.ascii_whitespace)) |
There was a problem hiding this comment.
suggestion: Consider renaming is_not_blank_line for clarity.
The variable name is_not_blank_line could be more descriptive. Consider renaming it to something like has_content or line_has_content to better convey its purpose.
| is_not_blank_line = bool(line_to_parse.strip(Constants.ascii_whitespace)) | |
| line_has_content = bool(line_to_parse.strip(Constants.ascii_whitespace)) |
| extra_consumed_whitespace, | ||
| container_level_tokens, | ||
| original_line, | ||
| is_not_blank_line, |
There was a problem hiding this comment.
question: Check if is_not_blank_line is necessary as a parameter.
Since is_not_blank_line is derived from line_to_parse, consider whether it is necessary to pass it as a parameter to __do_block_quote_leading_spaces_adjustments. It might be more efficient to compute it within the method itself.
| ) | ||
|
|
||
| @staticmethod | ||
| def __look_for_container_prefix( |
There was a problem hiding this comment.
suggestion (testing): New method __look_for_container_prefix.
The new method __look_for_container_prefix appears to be added for checking container prefixes. Ensure that this method is thoroughly tested for all edge cases, especially with different types of list and block quote combinations.
| def __look_for_container_prefix( | |
| @staticmethod | |
| def __look_for_container_prefix( | |
| token_stack: List[MarkdownToken], container_line: str | |
| ) -> bool: | |
| # Add thorough unit tests to cover edge cases | |
| # Test with different types of list and block quote combinations | |
| pass |
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_044ca(): |
There was a problem hiding this comment.
suggestion (testing): Missing edge case for empty block quote
Consider adding a test case where the block quote is empty or contains only whitespace. This will ensure that the parser handles such cases correctly.
| def test_extra_044ca(): | |
| @pytest.mark.gfm | |
| def test_extra_044ca(): | |
| """ | |
| Test for empty block quote | |
| """ | |
| input_text = "> " | |
| expected_output = "<blockquote>\n</blockquote>" | |
| assert parse_markdown(input_text) == expected_output |
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_extra_044fxa(): |
There was a problem hiding this comment.
suggestion (testing): Test case lacks description
Please provide a meaningful description for the test case. This will help future maintainers understand the purpose of the test.
| def test_extra_044fxa(): | |
| @pytest.mark.gfm | |
| def test_extra_044fxa(): | |
| """ | |
| Test case for verifying the handling of extra markdown features. | |
| """ |
| > > > -------- | ||
| """, | ||
| scan_expected_return_code=1, | ||
| scan_expected_output="""{temp_source_path}:2:7: MD031: Fenced code blocks should be surrounded by blank lines (blanks-around-fences) |
There was a problem hiding this comment.
suggestion (testing): Consider adding more edge cases
It would be beneficial to add more edge cases, such as when the fenced code block is at the start or end of the document, to ensure comprehensive coverage.
| scan_expected_output="""{temp_source_path}:2:7: MD031: Fenced code blocks should be surrounded by blank lines (blanks-around-fences) | |
| scan_expected_output="""{temp_source_path}:1:1: MD031: Fenced code blocks should be surrounded by blank lines (blanks-around-fences) | |
| {temp_source_path}:2:7: MD031: Fenced code blocks should be surrounded by blank lines (blanks-around-fences) | |
| {temp_source_path}:3:7: MD031: Fenced code blocks should be surrounded by blank lines (blanks-around-fences) | |
| {temp_source_path}:4:1: MD031: Fenced code blocks should be surrounded by blank lines (blanks-around-fences) | |
| """ |
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_nested_three_ordered_nl_block_skip_nl_block_skip_1(): |
There was a problem hiding this comment.
suggestion (testing): Test case lacks description
Please provide a meaningful description for the test case. This will help future maintainers understand the purpose of the test.
| def test_nested_three_ordered_nl_block_skip_nl_block_skip_1(): | |
| def test_nested_three_ordered_nl_block_skip_nl_block_skip_1(): | |
| """ | |
| Test nested structure with ordered list and block quotes. | |
| Ensures correct parsing and rendering of nested elements. | |
| """ |
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| def test_nested_three_ordered_nl_block_skip_nl_block_skip_2(): |
There was a problem hiding this comment.
suggestion (testing): Test case lacks description
Please provide a meaningful description for the test case. This will help future maintainers understand the purpose of the test.
| def test_nested_three_ordered_nl_block_skip_nl_block_skip_2(): | |
| def test_nested_three_ordered_nl_block_skip_nl_block_skip_2(): | |
| """ | |
| Test nested structure with ordered list and block quotes. | |
| Ensures correct parsing and rendering of nested elements. | |
| """ |
|
|
||
|
|
||
| @pytest.mark.gfm | ||
| @pytest.mark.skip |
There was a problem hiding this comment.
question (testing): Skipped test case
Please provide a reason for skipping this test case. If it's a known issue, consider linking to the relevant issue tracker.
| - check for adding extra line to list with blank line in *-List-Bq | ||
| not flexible enough |
There was a problem hiding this comment.
suggestion (documentation): Rephrase for clarity
Consider rephrasing to 'The check for adding an extra line to a list with a blank line in *-List-Bq is not flexible enough.'
| - check for adding extra line to list with blank line in *-List-Bq | |
| not flexible enough | |
| - The check for adding an extra line to a list with a blank line in *-List-Bq | |
| is not flexible enough |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 190 190
Lines 19960 19986 +26
Branches 2511 2517 +6
=========================================
+ Hits 19960 19986 +26 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
This pull request refactors the handling of block quotes and lists to better manage blank lines and leading spaces, adds new test cases to cover these scenarios, updates documentation, and adjusts test results and coverage reports accordingly.