-
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/801 #1058
Conversation
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: 7 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
and configure rules to correspond to what you want the Markdown guidelines | ||
to be for your project. | ||
|
||
## What Advantages Does PyMarkdown Have Over Other Markdown Linters? |
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 (code_clarification): Clarify unique advantages over competitors in the new section.
While the section on advantages is informative, consider highlighting unique features or performance metrics that clearly differentiate PyMarkdown from its competitors.
## What Advantages Does PyMarkdown Have Over Other Markdown Linters? | |
## Key Differentiators of PyMarkdown Compared to Other Markdown Linters | |
PyMarkdown not only adheres to standard Markdown linting practices but also excels with unique features such as: | |
- **Performance Optimization:** Ensures faster processing times through optimized algorithms. | |
- **Custom Rule Configuration:** Allows detailed customization to fit the specific needs of your project. |
for more information on the options available for use in your Pre-Commit Hooks. | ||
|
||
## Requirements | ||
## What Are The Minimum Requirements? |
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 (code_clarification): Validate the clarity and completeness of the new minimum requirements section.
Ensure that the new section on minimum requirements is clear and complete, possibly including more specific details about system requirements or dependencies.
## What Are The Minimum Requirements? | |
## Minimum System Requirements | |
To ensure optimal performance, this project requires: | |
- Python version: 3.8 or later | |
- Operating System: Compatible with Windows, macOS, and Linux | |
Please ensure your system meets these requirements before installation. |
* [ ] Documentation | ||
* [ ] Scan/Rule not working as expected | ||
* [ ] Fix/Rule not working as expected | ||
* [ ] Other |
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 (code_clarification): Clarify the 'Other' bug type.
It would be helpful to specify what 'Other' encompasses or provide an example to guide users in categorizing bugs that do not fit the predefined categories.
* [ ] Other | |
* [ ] Other (Please specify or provide an example if your issue does not fit the categories above) |
|
||
Things that will be added: | ||
|
||
- ensure solid documentation on each extensions |
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.
nitpick (typo): Typo in 'extensions'.
@@ -197,6 +197,7 @@ def test_api_scan_recursive_for_directory(): | |||
f"{docs_prefix}api-usage.md", | |||
f"{docs_prefix}api.md", | |||
f"{extensions_prefix}front-matter.md", | |||
f"{docs_prefix}old_README.md", |
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.
question (testing): Please verify the updated assertion for scan failures count.
The test case has been updated to expect 68 scan failures, up from 55. Ensure this change is justified by the addition of 'old_README.md' and that all new failure cases are intended and correctly handled.
@@ -277,6 +277,7 @@ def test_api_list_for_directory(): | |||
"developer.md", | |||
"extensions.md", | |||
"faq.md", | |||
"old_README.md", |
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 (testing): Consider adding a specific assertion for the inclusion of 'old_README.md'.
While the file 'old_README.md' has been added to the list of files, there's no specific assertion checking for its presence in the output. Adding such an assertion would ensure that the file is indeed listed as expected.
@@ -367,6 +367,7 @@ def test_markdown_with_dash_l_on_directory(): | |||
{source_path}developer.md | |||
{source_path}extensions.md | |||
{source_path}faq.md | |||
{source_path}old_README.md |
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 (testing): Ensure consistency in file listing tests.
The addition of 'old_README.md' in the test output should be accompanied by a corresponding assertion to check its presence, ensuring the test remains robust and reflective of actual functionality.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1058 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 189 189
Lines 19197 19197
Branches 2393 2393
=========================================
Hits 19197 19197 ☔ View full report in Codecov by Sentry. |
No description provided.