https://github.com/jackdewinter/pymarkdown/issues/1250 #1251
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 PR fixes issues with the MD031 rule (fenced code blocks should be surrounded by blank lines) when handling nested containers and container drops. The changes improve the handling of leading spaces and indentation in various container scenarios. Updated class diagram for PendingContainerAdjustmentclassDiagram
class PendingContainerAdjustment {
int insert_index
string leading_space_to_insert
bool do_insert
bool do_special
}
Updated class diagram for RuleMd031classDiagram
class RuleMd031 {
List~List~PendingContainerAdjustment~~ __container_adjustments
int __fix_count
Optional~MarkdownToken~ __removed_container_stack_token
List~MarkdownToken~ __x1
List~List~PendingContainerAdjustment~~ __x2
List~int~ __x3
Optional~List~PendingContainerAdjustment~~ __removed_container_adjustments
List~MarkdownToken~ __last_end_container_tokens
LeadingSpaceIndexTracker __leading_space_index_tracker
void starting_new_file()
void __fix_spacing_special_case(PluginScanContext context, MarkdownToken token)
void __fix_spacing_block_quote(MarkdownToken token, bool upgrade_kludge)
bool __fix_spacing_one_container(PluginScanContext context, MarkdownToken token, bool upgrade_kludge)
void __fix_spacing_with_fenced_and_list_end(PluginScanContext context, MarkdownToken token, MarkdownToken new_token)
void __fix_spacing_with_special_list_fix(PluginScanContext context, MarkdownToken token, MarkdownToken new_token)
void __fix_spacing_with_else(PluginScanContext context, MarkdownToken token, MarkdownToken new_token)
bool __calc_kludge_one(bool at_least_one_container)
bool __calc_2(PluginScanContext context, bool did_process_removals)
Tuple~bool, bool~ __calc_3(PluginScanContext context, bool did_process_removals, bool at_least_one_container, bool upgrade_kludge)
bool __apply_tailing_block_quote_fix(int modify_index, PluginScanContext context)
void __fix_spacing(PluginScanContext context, MarkdownToken token, bool special_case)
void __handle_fenced_code_block(PluginScanContext context, MarkdownToken token, bool special_case)
void __process_pending_container_end_adjustment(PluginScanContext context, List~PendingContainerAdjustment~ next_container_adjustment_list)
void __process_pending_container_end_tokens(PluginScanContext context, List~PendingContainerAdjustment~ next_container_adjustment_list)
void next_token(PluginScanContext context, MarkdownToken token)
}
Updated class diagram for TransformContainersclassDiagram
class TransformContainers {
static Optional~string~ __abcd_final_list_second_half(List~MarkdownToken~ removed_tokens, List~int~ removed_token_indices, List~MarkdownToken~ container_stack, Optional~string~ possible_prefix)
static Optional~string~ __abcd_final_list(List~MarkdownToken~ removed_tokens, List~int~ removed_token_indices, List~MarkdownToken~ container_stack)
static Optional~string~ __abcd_final(List~MarkdownToken~ removed_tokens, List~int~ removed_token_indices, List~MarkdownToken~ container_stack)
}
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 - here's some feedback:
Overall Comments:
- Consider refactoring the state tracking in rule_md_031.py to use a cleaner state machine design rather than multiple parallel lists (x1, x2, x3) for tracking container state
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -53,9 +54,13 @@ def __init__(self) -> None: | |||
| self.__container_adjustments: List[List[PendingContainerAdjustment]] = [] | |||
| self.__fix_count = 0 | |||
| self.__removed_container_stack_token: Optional[MarkdownToken] = None | |||
| self.__x1: List[MarkdownToken] = [] | |||
There was a problem hiding this comment.
suggestion: Consider using more descriptive names for __x1, __x2, and __x3 variables
These variables appear to represent removed tokens, container adjustments, and leading space indices respectively. More descriptive names would improve code readability and maintainability.
self.__removed_tokens: List[MarkdownToken] = []
| is_kludge_one = not any(i.is_block_quote_start for i in self.__x1[:-1]) | ||
| return is_kludge_one | ||
|
|
||
| def __calc_2(self, context: PluginScanContext, did_process_removals: bool) -> bool: |
There was a problem hiding this comment.
suggestion: Rename __calc_2 and __calc_3 methods to better describe their purpose
The current method names are not descriptive of their functionality. Consider names that indicate what these methods are calculating or processing.
def __check_list_indentation(self, context: PluginScanContext, did_process_removals: bool) -> bool:
test/test_markdown_extra.py
Outdated
| @pytest.mark.gfm | ||
| def test_extra_050c0(): | ||
| """ | ||
| TBD |
There was a problem hiding this comment.
issue (testing): Test descriptions are missing
Several test functions have 'TBD' as their docstring. Please add descriptive docstrings that explain what each test is verifying. This helps other developers understand the test cases and makes maintenance easier.
test/test_markdown_extra.py
Outdated
Comment on lines
9309
to
9310
| # Act & Assert | ||
| act_and_assert(source_markdown, expected_gfm, expected_tokens, show_debug=False) |
There was a problem hiding this comment.
suggestion (testing): Consider parameterizing similar tests
There are multiple similar test cases that differ mainly in their input data. Consider using pytest's parameterize feature to reduce code duplication and make it easier to add new test cases.
@pytest.mark.parametrize(
"source_markdown,expected_gfm,expected_tokens",
[
(source_markdown, expected_gfm, expected_tokens),
# Add more test cases here as needed
],
)
def test_markdown_parsing(source_markdown, expected_gfm, expected_tokens):
act_and_assert(source_markdown, expected_gfm, expected_tokens, show_debug=False)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1251 +/- ##
==========================================
- Coverage 99.98% 99.98% -0.01%
==========================================
Files 191 191
Lines 21123 21194 +71
Branches 2701 2715 +14
==========================================
+ Hits 21120 21190 +70
Misses 1 1
- Partials 2 3 +1 ☔ 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 #1250
Summary by Sourcery
Enhance the Markdown transformation logic to better handle spacing for fenced code blocks within nested lists and block quotes, and add comprehensive test cases to ensure coverage of these scenarios.
Enhancements:
Documentation:
Tests: