-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
https://github.com/jackdewinter/pymarkdown/issues/1267 #1279
Conversation
Reviewer's Guide by SourceryThis pull request fixes several issues related to block quotes, lists, and fenced code blocks in the PyMarkdown parser. The main changes involve improving the handling of container blocks, fixing assertion errors, and addressing edge cases in the parsing logic. Class diagram for MarkdownToken modificationsclassDiagram
class MarkdownToken {
+is_task_list() bool
+is_front_matter() bool
+is_text() bool
+_token_task_list : str
+_token_front_matter : str
+_token_block_quote : str
+_token_unordered_list_start : str
+_token_ordered_list_start : str
+_token_new_list_item : str
}
note for MarkdownToken "Added is_task_list method and reorganized token constants"
Class diagram for TransformContainers modificationsclassDiagram
class TransformContainers {
+__apply_line_transformation(..., ort: List<MarkdownToken>) str
+__abcd(..., container_line: str) Optional<str>
+__abcd_remaining(..., container_line: str, prefix: Optional<str>) Optional<str>
+__abcd_final_list_second_half(...)
+__abcd_final(...)
+__apply_container_transformation_inner(..., ort: List<MarkdownToken>) str
+__adjust_for_list(..., ort: List<MarkdownToken>) Tuple<str, bool>
}
note for TransformContainers "Added ort parameter and __abcd_remaining method"
Class diagram for InlineProcessor modificationsclassDiagram
class InlineProcessor {
+__parse_inline_tracker_start(lsi_tracker: LeadingSpaceIndexTracker, token: MarkdownToken) void
+__parse_inline_tracker_per_token(lsi_tracker: LeadingSpaceIndexTracker, token: MarkdownToken) void
+parse_inline(coalesced_results: List<MarkdownToken>, ...) void
}
note for InlineProcessor "Refactored parse_inline method with new helper methods"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: all looks good
- 🟢 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.
@@ -179,7 +179,7 @@ def remove_last_leading_space(self) -> Optional[str]: | |||
extracted_text = self.__leading_spaces | |||
self.__leading_spaces = None | |||
else: | |||
extracted_text = self.__leading_spaces[last_separator_index:] | |||
extracted_text = self.__leading_spaces[last_separator_index + 1 :] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Add bounds check for last_separator_index before adding 1
If last_separator_index is -1, adding 1 will still result in an invalid index. Consider adding a bounds check or handling this edge case explicitly.
newdocs/src/changelog.md
Outdated
- Fixed remaining assert issues, leaving fixes that produce valid | ||
Markdown, but not the intended Markdown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: The description of Issue 1274's fix could be clearer
Consider rewording to clarify what was fixed and what remains to be addressed. For example: 'Fixed assert issues while preserving Markdown validity, though some cases may still not produce the exact intended Markdown output.'
- Fixed remaining assert issues, leaving fixes that produce valid | |
Markdown, but not the intended Markdown. | |
- Fixed assert issues while preserving Markdown validity, though some cases may still not produce the exact intended Markdown output. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1279 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 191 191
Lines 21390 21393 +3
Branches 2751 2751
=========================================
+ Hits 21390 21393 +3 ☔ View full report in Codecov by Sentry. |
#1267
Summary by Sourcery
Enhance markdown transformation logic to better handle nested block quotes and lists, addressing parsing issues. Fix a bug related to task lists causing errors in rule MD018. Update tests to cover new scenarios and ensure robustness.
Bug Fixes:
Enhancements:
Tests: