https://github.com/jackdewinter/pymarkdown/issues/1247 #1248
Conversation
Reviewer's Guide by SourceryThis PR fixes an issue with list indentation calculation in cases where there are 3 or more nested lists without block quotes. The changes primarily focus on improving the handling of leading spaces and container adjustments in the markdown parser. Class diagram for TransformContainers and RuleMd031 changesclassDiagram
class TransformContainers {
+__abcd_final_list_second_half(removed_tokens: List<MarkdownToken>, removed_token_indices: List<int>, container_stack: List<MarkdownToken>, possible_prefix: Optional<str>) Optional<str>
+__abcd_final_list(removed_tokens: List<MarkdownToken>, removed_token_indices: List<int>, container_stack: List<MarkdownToken>) Optional<str>
}
class RuleMd031 {
+__fix_spacing_one_container(context: PluginScanContext, token: MarkdownToken) bool
+__fix_spacing_removed_container(context: PluginScanContext, token: MarkdownToken) void
+next_token(context: PluginScanContext, token: MarkdownToken) void
+__last_end_container_tokens: List<MarkdownToken>
}
TransformContainers --> RuleMd031 : uses
note for TransformContainers "Handles container adjustments and leading spaces"
note for RuleMd031 "Manages spacing fixes for markdown tokens"
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: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 2 issues found
- 🟡 Documentation: 3 issues found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
test/test_markdown_extra.py
Outdated
| @pytest.mark.skip | ||
| def test_extra_050a0(): |
There was a problem hiding this comment.
issue (testing): Test case is marked as skipped without explanation
This test case is marked with @pytest.mark.skip but there's no comment explaining why it's skipped. Consider adding a comment explaining the reason for skipping or remove the skip marker if the test should be running.
test/test_markdown_extra.py
Outdated
| """ | ||
| TBD |
There was a problem hiding this comment.
issue (testing): Test docstring is incomplete
Multiple test cases have 'TBD' as their docstring. The docstring should describe what aspect of the functionality is being tested. Consider updating these with proper descriptions that explain the test's purpose.
test/rules/test_md031.py
Outdated
| skip_fix_bad_markdown = True | ||
| skip_fix_asserts = True |
There was a problem hiding this comment.
issue (testing): Global test skip flags without documentation
These global flags control test skipping behavior but lack documentation explaining their purpose and when they should be used. Consider adding comments explaining what these flags control and why they exist.
newdocs/src/changelog.md
Outdated
| - None | ||
| - [Issue 1233](https://github.com/jackdewinter/pymarkdown/issues/1233) | ||
| - [Issue 1234](https://github.com/jackdewinter/pymarkdown/issues/1234) | ||
| - [Issue 1235](https://github.com/jackdewinter/pymarkdown/issues/1235) | ||
| - Adding more comprehensive "drop X" tests where multiple levels of | ||
| containers are involved, and then dropping one or more of those | ||
| containers in a single line. | ||
|
|
||
| <!--- pyml disable-next-line no-duplicate-heading--> |
There was a problem hiding this comment.
question (documentation): Issues 1233-1235 appear in both Added and Fixed sections
Consider clarifying why these issues appear in both sections, or consolidate them into the most appropriate section to avoid confusion.
newdocs/src/changelog.md
Outdated
| - [Issue 1233](https://github.com/jackdewinter/pymarkdown/issues/1233) | ||
| - [Issue 1234](https://github.com/jackdewinter/pymarkdown/issues/1234) | ||
| - [Issue 1235](https://github.com/jackdewinter/pymarkdown/issues/1235) | ||
| - Adding more comprehensive "drop X" tests where multiple levels of |
There was a problem hiding this comment.
suggestion (documentation): The term "drop X" could benefit from more context
Consider explaining what "drop X" refers to, as this might not be immediately clear to all readers.
| - Adding more comprehensive "drop X" tests where multiple levels of | |
| - Adding more comprehensive database column dropping tests where multiple levels of schema dependencies are involved |
newdocs/src/changelog.md
Outdated
| - [Issue 1233](https://github.com/jackdewinter/pymarkdown/issues/1233) | ||
| - [Issue 1234](https://github.com/jackdewinter/pymarkdown/issues/1234) | ||
| - [Issue 1235](https://github.com/jackdewinter/pymarkdown/issues/1235) | ||
| - Adding new "drop_x" tests and resolve any scan issues with them. |
There was a problem hiding this comment.
nitpick (documentation): Inconsistent terminology between "drop X" and "drop_x"
Consider using consistent terminology throughout the changelog to avoid confusion.
| @@ -426,9 +426,55 @@ def __abcd( | |||
|
|
|||
| # pylint: enable=too-many-arguments, too-many-boolean-expressions | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
issue (complexity): Consider consolidating the indent calculation logic into a single method without stack manipulation.
The split into __abcd_final_list_second_half has introduced unnecessary complexity through stack manipulation and branching. Consider simplifying by focusing on the indent calculation:
@staticmethod
def __abcd_final_list(
removed_tokens: List[MarkdownToken],
removed_token_indices: List[int],
container_stack: List[MarkdownToken],
) -> Optional[str]:
# Handle list token prefix
removed_list_token = cast(ListStartMarkdownToken, removed_tokens[0])
assert removed_list_token.leading_spaces is not None
split_leading_spaces = removed_list_token.leading_spaces.split("\n")
possible_prefix = None
if removed_token_indices[0] < len(split_leading_spaces):
possible_prefix = split_leading_spaces[removed_token_indices[0]]
del removed_tokens[0]
del removed_token_indices[0]
# Calculate indent if only list tokens
if not any(t.is_block_quote_start for t in container_stack + removed_tokens):
current_indent_level = (container_stack[-1].indent_level
if container_stack else 0)
return (" " * current_indent_level if possible_prefix is None
else possible_prefix)
# Handle block quote case
if removed_tokens[0].is_block_quote_start:
bq_token = cast(BlockQuoteMarkdownToken, removed_tokens[0])
assert bq_token.bleading_spaces is not None
split_spaces = bq_token.bleading_spaces.split("\n")
assert removed_token_indices[0] < len(split_spaces)
prefix = "" if possible_prefix is None else possible_prefix
prefix = f"{split_spaces[removed_token_indices[0]]}{prefix}"
del removed_tokens[0]
del removed_token_indices[0]
return prefix
return NoneThis approach:
- Eliminates the need for stack copying and reversal
- Simplifies the branching logic
- Keeps the indent calculation separate from block quote handling
- Maintains all functionality while reducing state manipulation
| @@ -56,6 +56,7 @@ def __init__(self) -> None: | |||
| self.__removed_container_adjustments: Optional[ | |||
| List[PendingContainerAdjustment] | |||
| ] = None | |||
| self.__last_end_container_tokens: List[MarkdownToken] = [] | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring container end token handling to use a recursive traversal approach instead of maintaining state
The current implementation maintains ongoing state via __last_end_container_tokens and uses complex index manipulation to process them. This can be simplified while keeping the enhanced multi-container functionality:
def __collect_preceding_container_ends(self, token: MarkdownToken) -> List[MarkdownToken]:
"""Collect container end tokens that immediately precede this token."""
container_ends = []
current = self.__last_token
while current and (current.is_list_end or current.is_block_quote_end):
container_ends.insert(0, current)
current = self.__second_last_token
return container_ends
# In the fenced code block handling section:
if token.is_fenced_code_block and self.__last_token.is_list_end:
container_ends = self.__collect_preceding_container_ends(token)
replacement_tokens = [
BlankLineMarkdownToken(
extracted_whitespace="",
position_marker=PositionMarker(new_token.line_number - 1, 0, ""),
column_delta=1,
)
]
replacement_tokens.extend(container_ends)
replacement_tokens.append(new_token)
self.register_replace_tokens_request(
context, container_ends[0], token, replacement_tokens
)This approach:
- Eliminates the need for ongoing token list maintenance
- Computes container ends only when needed
- Removes complex index manipulation
- Keeps the enhanced multi-container functionality
|
|
||
| new_token = copy.deepcopy(token) | ||
| self.__fix_count += 1 | ||
| new_token.adjust_line_number(context, self.__fix_count) | ||
| assert self.__last_token is not None | ||
| if token.is_fenced_code_block and self.__last_token.is_list_end: | ||
| end_container_index = len(self.__last_end_container_tokens) - 1 |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
- Coverage 99.99% 99.98% -0.01%
==========================================
Files 191 191
Lines 21094 21123 +29
Branches 2696 2701 +5
==========================================
+ Hits 21092 21120 +28
Misses 1 1
- Partials 1 2 +1 ☔ View full report in Codecov by Sentry. |
closes #1247
Summary by Sourcery
Add new tests for complex nested Markdown structures and fix indentation issues in nested lists without block quotes. Refactor token handling logic to improve prefix determination and update the changelog with these changes.
New Features:
Bug Fixes:
Enhancements:
__abcd_final_listfunction to improve handling of removed tokens and container stacks, enhancing the logic for prefix determination.Documentation:
Tests: