- Notifications
You must be signed in to change notification settings - Fork407
feat: add pre-commit-hooks+config to use with pre-commit.com#1879
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
And self-apply in github workflows.
Uh oh!
There was an error while loading.Please reload this page.
| # Github image should be /usr/bin/pwsh | ||
| # Linux/Macos is usually /usr/local/bin/pwsh unless using a user scope | ||
| entry: /usr/bin/pwsh -Command "Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Severity Warning -Path . " | ||
| # https://github.com/pre-commit/pre-commit/pull/2340#issuecomment-1098203344 |
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.
shouldn't this just be removed?
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.
If removed, should be somewhere in docs (where?) as user may need to adapt powershell path
bergmeister 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.
I don't mind adding a CI enforcement check but against commit hooks using commit hooks because PSSA is a binary module and this would mean I'd need a separate shell for committing versus local testing.
Current CI setup seems to not be working anyway because when you run invoke-scriptanalyzer on repo like you do there ARE violations (which would need to be fixed first) , even when excluding theTests folder, which would be expected as well as running the default set of rules and not just the PSGallery ones.
juju4 commentedSep 17, 2023
It's up to you if want CI with warning(continue-on-error) or not and if violation fixes should go into this PR too. if small or exclusions, certainly. if bigger, probably better separate. Only warnings here but exit code is equivalent to error. |
leandroscardua commentedNov 27, 2024
Hi There, Do we have any news about it? |
andyleejordan commentedMar 5, 2025
My question is: why do we have to host it in this repo? Can't people just write their own commit hooks? I don't want to sign onto maintaining a configuration for some random service 🤷 |
juju4 commentedMar 9, 2025
imho, that's more a distribution channel than a configuration and pre-commit is probably not a random service, at least for developers. |
bergmeister commentedMar 13, 2025
I am with you on that. I think we are open to improvements of our developer experience but ultimately the amount of external contributions is relatively low to not warrant this unless more people vote on wanting this, so far there is just one reaction on this PR. |
And self-apply in github workflows.
PR Summary
This allows easy integration withpre-commit that can be use to validate/lint code before commit (and also after/server side as per github workflow example).
Tested on Macos and Linux.
Note that current workflow fail is related to warnings as seen onhttps://github.com/juju4/PSScriptAnalyzer/actions/runs/3867323454/jobs/6592019858#step:5:3554
can change severity to errors if prefer.
Further improvement discussion inpre-commit/pre-commit#2645
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.