Skip to content

Issue 1015#1050

Merged
jackdewinter merged 6 commits intomainfrom
issue-1015
May 26, 2024
Merged

Issue 1015#1050
jackdewinter merged 6 commits intomainfrom
issue-1015

Conversation

@jackdewinter
Copy link
Owner

@jackdewinter jackdewinter commented Mar 30, 2024

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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 found some issues that need to be addressed.

Blocking issues:

  • Assert statement left in production code. (link)
Here's what I looked at during the review
  • 🔴 General issues: 1 blocking issue
  • 🟢 Security: all looks good
  • 🟡 Testing: 3 issues found
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

original_line=original_line,
alternate_list_leading_space=alternate_leading_space,
)
# assert False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Assert statement left in production code.

It appears an assert False statement has been left in the code, likely for debugging purposes. This will raise an AssertionError every time this code path is executed, which could disrupt normal operation. Please remove or comment it out if it's no longer needed.

TBD
"""

# Arrange
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (testing): Removal of test cases reduces coverage for block quotes and lists.

The deletion of these test cases seems to remove coverage for specific markdown scenarios involving block quotes and lists. Please ensure that the removal is intentional and consider adding alternative tests if these scenarios are still relevant.

fix_expected_file_contents="""item 1
""",
),
pluginRuleTest(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): New test case for issue-1015 does not cover all edge cases.

The new test case 'issue-1015-positive' checks for hard tabs but does not seem to cover variations such as tabs in different contexts or mixed with spaces. Consider adding more comprehensive test scenarios to ensure robustness.

fix_expected_file_contents="""item 1
""",
),
pluginRuleTest(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Consider adding a positive test case without disabling MD010.

The 'issue-1015-negative' test case disables MD010 to test for MD047. Adding a positive test case that does not disable MD010 could help ensure that MD010 works as expected in conjunction with other rules.

@codecov
Copy link

codecov bot commented Mar 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (dc56962) to head (847d473).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1050   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          189       189           
  Lines        19197     19207   +10     
  Branches      2393      2394    +1     
=========================================
+ Hits         19197     19207   +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jackdewinter jackdewinter merged commit e4ef5c6 into main May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant