-
-
Notifications
You must be signed in to change notification settings - Fork 72
Add pre-commit hook to sort proselint config #892
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
base: main
Are you sure you want to change the base?
Add pre-commit hook to sort proselint config #892
Conversation
|
Thanks for your first pull request! We appreciate your contribution. |
Summary of ChangesHello @Subham-KRLX, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances development workflow by integrating a pre-commit hook designed to maintain the alphabetical order of keys within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Here's the code health analysis summary for commits Analysis Summary
|
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.
Code Review
This pull request introduces a useful pre-commit hook to automatically sort the proselint configuration file, which will help with maintainability. The implementation is straightforward, but I've identified a potential issue in the script that handles the sorting. The use of a hardcoded temporary file is unsafe and could lead to conflicts. I've provided a suggestion to make this more robust. Otherwise, the changes are good.
Co-authored-by: gemini-code-assist[bot]
jbampton
left a comment
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.
Do not change the proselint config by adding keys.
This is just to sort it
The config file already exists and should not be modified. This PR only adds the pre-commit hook to sort it.
jbampton
left a comment
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.
Don't delete files
Keep the original config with 7 checks, just sort the keys
|
@jbampton Fixed! I have restored the proselint-config.json file. |
jbampton
left a comment
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.
Can we sort but also maintain the same indentation ?
|
@jbampton Fixed Updated the |
jbampton
left a comment
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.
We need jq to indent with tabs
|
Can you fix the conflicts ? |
|
/gemini review |
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.
Code Review
This pull request adds a pre-commit hook to automatically sort the proselint-config.json file, which is a great way to ensure consistent formatting. The implementation is straightforward. My review includes a suggestion to make the hook's implementation more idiomatic for pre-commit and improve its maintainability by avoiding a hardcoded file path in the script. This small change will make the hook more robust.
.pre-commit-config.yaml
Outdated
| entry: bash -c 'f=.github/linters/proselint-config.json; jq -S --tab . "$f" > "$f.tmp" && mv "$f.tmp" "$f"' | ||
| language: system | ||
| files: ^\.github/linters/proselint-config\.json$ | ||
| pass_filenames: false |
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.
The current implementation with a hardcoded file path in the entry script works, but it's not idiomatic for pre-commit hooks and could be less maintainable. A better approach is to let pre-commit pass the filename to the script. This avoids duplicating the file path and makes the hook more reusable.
By removing pass_filenames: false, pre-commit will pass the matched filename as an argument ($1) to the entry script. This change makes the hook's configuration cleaner and easier to maintain if the file is ever renamed.
Additionally, since this hook has a system dependency on jq, it would be helpful to add a note about this requirement in the repository's contribution guidelines (e.g., CONTRIBUTING.md) for other contributors.
entry: bash -c 'jq -S --tab . "$1" > "$1.tmp" && mv "$1.tmp" "$1"' --
language: system
files: ^\.github/linters/proselint-config\.json$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.
Please make this a manual hook since bash generally does not run on Windows
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.
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.
"scripts": {
"format": "prettier --write "/*.{css,js,json,md,yaml,yml}"",
"check": "prettier --check "/*.{css,js,json,md,yaml,yml}"", "lint": "eslint --config .github/linters/eslint.config.mjs .",
"lint:fix": "eslint --config .github/linters/eslint.config.mjs . --fix",
}

Fixes #883
Description
Added a pre-commit hook that automatically sorts the proselint config JSON file using
jq -S. This ensures the configuration keys remain alphabetically sorted for easier maintenance.Changes
.github/linters/proselint-config.jsonwith sorted proselint checks.pre-commit-config.yamlto auto-sort JSON keysTesting
pre-commit run --all-files