ci: check for changes to lints separate from writing changes#2117
ci: check for changes to lints separate from writing changes#2117
Conversation
zimeg
added
tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2117 +/- ##
=======================================
Coverage 91.66% 91.66%
=======================================
Files 38 38
Lines 10317 10317
Branches 647 647
=======================================
Hits 9457 9457
Misses 848 848
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. |
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
📝 Leaving a few notes for the kind reviewers!
I was running into interesting strangeness in the CI matrix and left a note on changes from that 👁️🗨️
| - name: Configure git settings (Windows) | ||
| if: matrix.os == 'windows-latest' | ||
| run: | | ||
| git config --global core.autocrlf false | ||
| git config --global core.eol lf |
There was a problem hiding this comment.
This step is added to not convert files to ending lines with \r\n after a checkout.
Some searching hints at .gitattributes being an option for converting files to \n endings before a commit, which might be an option if lintings begin to fail due to files being committed with \r\n!
There was a problem hiding this comment.
is there a reason why the files' ending lines are converted to this? I'm guessing we're getting rid of those ending lines because it's clashing with the linter's parsing?
There was a problem hiding this comment.
It seems to be a Windows default as far as I can tell 🤔
Some suggestions add the following to .gitattributes to prefer lf but that might break editing experiences on certain setups that expect the \r\n:
* text eol=lf
And some repos treat text files as binary to never change endings on checkout:
* -text
That last example is so interesting to me. The linter does expect \n in all cases and was erroring if that wasn't found, and I'm hoping this change in CI is an alright enough workaround for matching expected line endings 👾
| "lint": "npx @biomejs/biome check .", | ||
| "lint:fix": "npx @biomejs/biome check --write .", |
There was a problem hiding this comment.
This change is matched across all packages using biome 🙏
| "*": [ | ||
| "./types/*" | ||
| ] | ||
| "*": ["./types/*"] |
There was a problem hiding this comment.
Other changes are made to fix errors from npm run lint 🔍
There was a problem hiding this comment.
what error would be thrown without this change?
There was a problem hiding this comment.
$ npx @biomejs/biome check .
./tsconfig.json format ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
✖ Formatter would have printed the following content:
17 17 │ "baseUrl": ".",
18 18 │ "paths": {
19 │ - ······"*":·[
20 │ - ········"./types/*"
21 │ - ······]
19 │ + ······"*":·["./types/*"]
22 20 │ },
23 21 │ "esModuleInterop": true
····· │
27 25 │ // "resolveJsonModule": true,
28 26 │ },
29 │ - ··"include":·[
30 │ - ····"src/**/*"
31 │ - ··],
32 │ - ··"exclude":·[
33 │ - ····"src/**/*.spec.*"
34 │ - ··],
27 │ + ··"include":·["src/**/*"],
28 │ + ··"exclude":·["src/**/*.spec.*"],
35 29 │ "jsdoc": {
36 30 │ "out": "support/jsdoc",
Checked 9 files in 3ms. No fixes applied.
Found 1 error.
check ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
✖ Some errors were emitted while running checks.
An expection of an inlined list. What a beautiful error message this is 🤧
My output even shows diff with red and green 🚙 💨 TBH I would've kept the separate lines but I agree with the linter 🙏
There was a problem hiding this comment.
This is even an error that was changed with automagic 🪄 Quite impressive! ✨
hello-ashleyintech
left a comment
hello-ashleyintech
left a comment
There was a problem hiding this comment.
PR generally LGTM, thanks for doing this! 🙇 left a few questions mostly for curiosity purposes
| "*": [ | ||
| "./types/*" | ||
| ] | ||
| "*": ["./types/*"] |
There was a problem hiding this comment.
what error would be thrown without this change?
|
@hello-ashleyintech 🙏 Thank you for another fast review! I'm glad a few updates from the linter were caught here, and I'm wondering if future updates from the great @dependabot might cause errors here? 👁️🗨️ For now, I think we're alright to merge these changes, but open to following up with other changes around line endings or similar! 🚢 |
2 participants
Summary
This PR adds the
lint:fixjob to write changes needed for linting to pass. Thenpm run lintcommand is changed to just check for changes. That'll cause CI to fail if changes are needed!This matches the changes discussed in slack-samples/bolt-ts-starter-template#131 and follows slackapi/bolt-js#2357.
Preview
Before changes
https://github.com/slackapi/node-slack-sdk/actions/runs/12128359302/job/33814593433#step:8:11
After changes
Found in the action logs of this PR!
Requirements