-
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/1322 #1323
Conversation
Reviewer's Guide by SourceryThis pull request focuses on improving the project's shell scripts by adding a linter and formatter to the pre-commit hooks, and updating the scripts to be more robust. Flow diagram for updated shell script validation processgraph TD
A[Shell Script] --> B{Pre-commit hooks}
B --> C[ShFmt Formatter]
B --> D[Shellcheck Linter]
C --> E[Format shell script]
D --> F[Validate shell script]
E --> G[Formatted script]
F --> |Pass| H[Valid script]
F --> |Fail| I[Validation errors]
style B fill:#f9f,stroke:#333,stroke-width:2px
File-Level Changes
Assessment against linked issues
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 found some issues that need to be addressed.
Blocking issues:
- Hardcoded secret
set SOURCERY_USER_KEY=0123456789abcdef0123456789abcdef01234567
should not be committed to the repository. (link)
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 1 blocking issue
- 🟢 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.
fi | ||
|
||
pipenv run python $PYTHON_PERFORMANCE_ARGUMENTS "${SCRIPT_DIR}/main.py" ${PARAMS[*]} | ||
pipenv run python ${PYTHON_PERFORMANCE_ARGUMENTS} "${SCRIPT_DIR}/main.py" "${PARAMS[@]}" |
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: Use "${array[@]}" consistently for array expansion across all scripts
While ${array[*]} works in many cases, "${array[@]}" handles spaces in parameters more reliably. Consider standardizing on [@] across all scripts.
Suggested implementation:
complete_process "${EXIT_CODE}"
The developer should also review any other scripts in the project for similar array expansions using ${array[*]}
and standardize them to use "${array[@]}"
for consistency.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 191 191
Lines 21590 21590
Branches 3991 3991
=========================================
Hits 21590 21590 ☔ View full report in Codecov by Sentry. |
#1322
closes #1322
Summary by Sourcery
Format shell scripts with shfmt, and lint them with shellcheck.
Build:
CI: