https://github.com/jackdewinter/pymarkdown/issues/1099 #1100
Conversation
![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: 7 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -116,6 +118,7 @@ def __build_backtick_response( | |||
| original_between_text, | |||
| leading_whitespace, | |||
| trailing_whitespace, | |||
| inline_response.adj_newlines, | |||
There was a problem hiding this comment.
issue (bug_risk): Potential issue with uninitialized variable
It appears that inline_response.adj_newlines is being used without being initialized in some cases. Ensure that adj_newlines is properly initialized in all code paths to avoid potential issues.
| @@ -196,10 +199,43 @@ def __backtick_split_lines(input_string: str) -> List[str]: | |||
| split_array.append(extracted_text) | |||
| return split_array | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
suggestion: Consider adding a docstring
Adding a docstring to the __adjust_for_injected_noops method would improve code readability and maintainability by explaining its purpose and usage.
| @@ -288,9 +503,27 @@ def __calculate_backtick_between_tabified_text( | |||
| assert ( | |||
| inline_request.tabified_text is not None | |||
| ), "tabified_text must be defined by now." | |||
| adj_tabified_text, whitespace_to_add, did_use_para_space, adj_newlines = ( | |||
| InlineBacktickHelper.__calc_tabified_text( | |||
There was a problem hiding this comment.
suggestion: Consider renaming method for clarity
The method name __calc_tabified_text could be more descriptive. Consider renaming it to something like __calculate_tabified_text_with_whitespace to better convey its purpose.
| InlineBacktickHelper.__calc_tabified_text( | |
| InlineBacktickHelper.__calculate_tabified_text_with_whitespace( |
| ) | ||
| if adj_newlines: |
There was a problem hiding this comment.
suggestion (performance): Potential performance impact
The check for adj_newlines and subsequent increment of newlines_encountered could be optimized. Consider if this logic can be simplified or combined with other newline handling logic to improve performance.
| if adj_newlines: | |
| newlines_encountered += adj_newlines |
| @@ -139,7 +139,9 @@ def __rehydrate_inline_code_span( | |||
| return "" | |||
|
|
|||
| current_inline_token = cast(InlineCodeSpanMarkdownToken, current_token) | |||
| span_text = ParserHelper.remove_all_from_text(current_inline_token.span_text) | |||
| span_text = ParserHelper.remove_all_from_text( | |||
There was a problem hiding this comment.
suggestion: Clarify the purpose of include_noops parameter
The include_noops parameter in ParserHelper.remove_all_from_text is not immediately clear. Consider adding a comment or docstring to explain its purpose and usage.
| span_text = ParserHelper.remove_all_from_text( | |
| # The `include_noops` parameter ensures that no-op characters are also removed | |
| span_text = ParserHelper.remove_all_from_text( | |
| current_inline_token.span_text, include_noops=True | |
| ) |
| @@ -32,6 +32,8 @@ class InlineRequest: | |||
| tabified_text: Optional[str] = None | |||
| parse_properties: Optional[ParseBlockPassProperties] = None | |||
| last_container_token: Optional[MarkdownToken] = None | |||
| whitespace_to_recombine: Optional[str] = None | |||
There was a problem hiding this comment.
issue (bug_risk): Consider initializing new attributes
The new attributes whitespace_to_recombine and para_space are added to InlineRequest. Ensure these are initialized properly in the constructor or other initialization methods to avoid potential NoneType errors.
| @@ -24,6 +24,7 @@ class InlineResponse: | |||
| delta_line_number: int = 0 | |||
| delta_column_number: int = 0 | |||
| reduce_remaining_line_by: int = 0 | |||
| adj_newlines: int = 0 | |||
There was a problem hiding this comment.
suggestion: Consider adding a comment for new attribute
Adding a comment or docstring for the new adj_newlines attribute in InlineResponse would help other developers understand its purpose and usage.
| adj_newlines: int = 0 | |
| # Number of adjacent newlines encountered | |
| adj_newlines: int = 0 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1100 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 189 189
Lines 19305 19406 +101
Branches 2401 2415 +14
==========================================
+ Hits 19305 19406 +101 ☔ View full report in Codecov by Sentry. |
Fixing long standing disabled test.