Skip to content

ci: check for changes to lints separate from writing changes #2117

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.

Sign up for GitHub

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

Merged
zimeg merged 7 commits into from
Dec 5, 2024
Merged

ci: check for changes to lints separate from writing changes #2117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a97713d
ci: check for changes to lints separate from writing changes
zimeg Dec 5, 2024
87fc008
style: format json according to linter preferences
zimeg Dec 5, 2024
c19e7c0
style: run the lint:fix job with the latest biome installed
zimeg Dec 5, 2024
e8c18d8
ci: configure git to checkout with eol lf on windows
zimeg Dec 5, 2024
24d6b7d
ci: configure the global git setting before checkout
zimeg Dec 5, 2024
fa72a4e
style: match naming conventions for os steps
zimeg Dec 5, 2024
b4c57d2
ci: avoid converting crlf on windows checkout
zimeg Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/ci-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ jobs:
- webhook
runs-on: ${{ matrix.os }}
steps:
- name: Configure git settings (Windows)
if: matrix.os == 'windows-latest'
run: |
git config --global core.autocrlf false
git config --global core.eol lf
Comment on lines +29 to +33
Copy link
Member Author

@zimeg zimeg Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@hello-ashleyintech hello-ashleyintech Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@zimeg zimeg Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👾

- uses: actions/checkout@v4
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
Expand Down
3 changes: 2 additions & 1 deletion packages/cli-hooks/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@
"prebuild": "shx rm -rf ./coverage",
"build": "shx chmod +x src/*.js",
"prelint": "tsc --noemit --module es2022 --maxNodeModuleJsDepth 0 --project ./jsconfig.json",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
Comment on lines +37 to +38
Copy link
Member Author

@zimeg zimeg Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is matched across all packages using biome 🙏

"pretest": "npm run lint",
"test": "c8 mocha src/*.spec.js"
},
Expand Down
3 changes: 2 additions & 1 deletion packages/cli-test/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@
"url": "https://github.com/slackapi/node-slack-sdk/issues"
},
"scripts": {
"lint": "npx @biomejs/biome check --write .",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist ./coverage",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"prepare": "npm run build",
"mocha": "cross-env SLACK_CLI_PATH=/doesnt/matter mocha --config .mocharc.json src/*.spec.ts src/**/*.spec.ts src/**/**/*.spec.ts",
"test": "npm run lint && npm run build && c8 npm run mocha"
Expand Down
3 changes: 2 additions & 1 deletion packages/logger/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist ./coverage",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"mocha": "mocha --config .mocharc.json src/*.spec.js",
"test:unit": "npm run build && npm run mocha",
"test": "npm run lint && npm run coverage",
Expand Down
12 changes: 3 additions & 9 deletions packages/logger/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,16 @@
"moduleResolution": "node",
"baseUrl": ".",
"paths": {
"*": [
"./types/*"
]
"*": ["./types/*"]
Copy link
Member Author

@zimeg zimeg Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other changes are made to fix errors from npm run lint 🔍

Copy link
Contributor

@hello-ashleyintech hello-ashleyintech Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what error would be thrown without this change?

Copy link
Member Author

@zimeg zimeg Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$ 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 🙏

Copy link
Member Author

@zimeg zimeg Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is even an error that was changed with automagic 🪄 Quite impressive! ✨

},
"esModuleInterop": true
// Not using this setting because its only used to require the package.json file, and that would change the
// structure of the files in the dist directory because package.json is not located inside src. It would be nice
// to use import instead of require(), but its not worth the tradeoff of restructuring the build (for now).
// "resolveJsonModule": true,
},
"include": [
"src/**/*"
],
"exclude": [
"src/**/*.spec.*"
],
"include": ["src/**/*"],
"exclude": ["src/**/*.spec.*"],
"jsdoc": {
"out": "support/jsdoc",
"access": "public"
Expand Down
3 changes: 2 additions & 1 deletion packages/oauth/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist ./coverage",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"test": "npm run lint && npm run coverage",
"coverage": "npm run build && c8 npm run test:mocha",
"test:mocha": "mocha --config .mocharc.json src/*.spec.ts src/**/*.spec.ts",
Expand Down
13 changes: 3 additions & 10 deletions packages/oauth/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,16 @@
"moduleResolution": "node",
"baseUrl": ".",
"paths": {
"*": [
"./types/*"
]
"*": ["./types/*"]
},
"esModuleInterop": true
// Not using this setting because its only used to require the package.json file, and that would change the
// structure of the files in the dist directory because package.json is not located inside src. It would be nice
// to use import instead of require(), but its not worth the tradeoff of restructuring the build (for now).
// "resolveJsonModule": true,
},
"include": [
"src/**/*"
],
"exclude": [
"src/**/spec-utils.ts",
"src/**/*.spec.*"
],
"include": ["src/**/*"],
"exclude": ["src/**/spec-utils.ts", "src/**/*.spec.*"],
"jsdoc": {
"out": "support/jsdoc",
"access": "public"
Expand Down
3 changes: 2 additions & 1 deletion packages/rtm-api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"test": "npm run lint && npm run build && npm run test:integration",
"test:integration": "mocha --config .mocharc.json test/integration.spec.js"
},
Expand Down
12 changes: 3 additions & 9 deletions packages/rtm-api/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,16 @@
"moduleResolution": "node",
"baseUrl": ".",
"paths": {
"*": [
"./types/*"
]
"*": ["./types/*"]
},
"esModuleInterop": true
// Not using this setting because its only used to require the package.json file, and that would change the
// structure of the files in the dist directory because package.json is not located inside src. It would be nice
// to use import instead of require(), but its not worth the tradeoff of restructuring the build (for now).
// "resolveJsonModule": true,
},
"include": [
"src/**/*"
],
"exclude": [
"src/**/*.spec.*"
],
"include": ["src/**/*"],
"exclude": ["src/**/*.spec.*"],
"jsdoc": {
"out": "support/jsdoc",
"access": "public"
Expand Down
3 changes: 2 additions & 1 deletion packages/socket-mode/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist ./coverage",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"test:unit": "mocha --config .mocharc.json src/**/*.spec.ts",
"test:coverage": "c8 npm run test:unit",
"test:integration": "mocha --config .mocharc.json test/integration.spec.js",
Expand Down
3 changes: 2 additions & 1 deletion packages/types/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"test": "npm run lint && npm run build && npm run test:types",
"test:types": "tsd"
},
Expand Down
3 changes: 2 additions & 1 deletion packages/web-api/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist ./coverage",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"mocha": "mocha --config .mocharc.json \"./src/**/*.spec.ts\"",
"test": "npm run lint && npm run test:types && npm run test:integration && npm run test:unit",
"test:integration": "npm run build && node test/integration/commonjs-project/index.js && node test/integration/esm-project/index.mjs && npm run test:integration:ts",
Expand Down
3 changes: 2 additions & 1 deletion packages/webhook/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"prepare": "npm run build",
"build": "npm run build:clean && tsc",
"build:clean": "shx rm -rf ./dist ./coverage",
"lint": "npx @biomejs/biome check --write .",
"lint": "npx @biomejs/biome check .",
"lint:fix": "npx @biomejs/biome check --write .",
"mocha": "mocha --config .mocharc.json src/*.spec.ts",
"test": "npm run lint && npm run test:unit",
"test:unit": "npm run build && c8 npm run mocha"
Expand Down
12 changes: 3 additions & 9 deletions packages/webhook/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,16 @@
"moduleResolution": "node",
"baseUrl": ".",
"paths": {
"*": [
"./types/*"
]
"*": ["./types/*"]
},
"esModuleInterop": true
// Not using this setting because its only used to require the package.json file, and that would change the
// structure of the files in the dist directory because package.json is not located inside src. It would be nice
// to use import instead of require(), but its not worth the tradeoff of restructuring the build (for now).
// "resolveJsonModule": true,
},
"include": [
"src/**/*"
],
"exclude": [
"src/**/*.spec.*"
],
"include": ["src/**/*"],
"exclude": ["src/**/*.spec.*"],
"jsdoc": {
"out": "support/jsdoc",
"access": "public"
Expand Down
Loading