Skip to content
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

chore: add scripts to format/lint only the delta files #305

Merged
merged 1 commit into from
Jan 30, 2024
Merged

chore: add scripts to format/lint only the delta files #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 .eslintignore
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file conflicts with the ignorePatterns section of .eslintrc.cjs, and it's producing warnings

I added ignorePatterns: ['!/.*'] to ensure .eslintrc.cjs itself would be linted, but that made the assumption ESLint itself was only ever going to be used with . as its argument. That pattern needs to be more specific now

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
scripts/*
.eslintignore
.prettierignore
.github/workflows/*
*.md
2 changes: 1 addition & 1 deletion .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module.exports = {
'plugin:svelte/recommended',
'prettier',
],
plugins: ['svelte', 'simple-import-sort'],
plugins: ['svelte', 'simple-import-sort', 'json-files'],
rules: {
'simple-import-sort/imports': 'error',
},
Expand Down
3 changes: 3 additions & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
scripts/*
.eslintignore
.prettierignore
8 changes: 4 additions & 4 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ The module is released automatically from the `main` branch using [semantic-rele

## Development setup

After cloning the repository, use the `setup` script to install development dependencies and the `validate` script to run all checks and tests to verify your setup.
After cloning the repository, install the project's dependencies and run the `validate` script to run all checks and tests to verify your setup.

```shell
npm run setup
npm install # or `pnpm install`, or `yarn install`, etc.
npm run validate
```

Expand All @@ -27,13 +27,13 @@ npm run validate
Run auto-formatting to ensure any changes adhere to the code style of the repository:

```shell
npm run format
npm run format:delta
```

To run lint and format checks without making any changes:

```shell
npm run lint
npm run lint:delta
```

### Test
Expand Down
11 changes: 9 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,19 @@
],
"scripts": {
"toc": "doctoc README.md",
"lint": "(prettier . --check || true) && eslint .",
"lint": "prettier . --check && eslint .",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If "unformatted files exist" is an expected state of this repository, then lint shouldn't error if there are unformatted files; I think the || true should be added back to prettier to downgrade it back to warnings

"lint:delta": "npm-run-all -p prettier:delta eslint:delta",
"prettier:delta": "prettier --check `./scripts/changed-files`",
"eslint:delta": "eslint `./scripts/changed-files`",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can understand the rationale behind incremental prettier, but incremental ESLint goes a step too far in my book. ESLint in our config has most of its formatting checks disabled via eslint-config-prettier, which means its mostly running code quality checks1 ("this should be a const", "var is not in scope," etc.)

Why go out of our way to avoid catching non-stylistic code quality issues?

Footnotes

  1. except for the import ordering plugin, but that doesn't feel very high risk for churn given its simplicity / consistency ↩

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can understand the rationale behind incremental prettier, but incremental ESLint goes a step too far in my book.

Yes, you would do things differently. I got that the first time around.

Listen, we just went through that discussion. At this point in time, I will not have something that force changing large sway of code to appease automatic checks on formatting or linting. Nobody force you to use those delta scripts. You are quite welcome and encouraged to have new code conform to the lint checks. But, and I'll repeat again to be clear, at this point in time I will not have something that force changing large sway of code to appease automatic checks on formatting or linting.

Now, can we consider this particular discussion done, and move to other things?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My apologies, I got a little unproductively worked up. I get a little passionate about testing practices 😅

"format": "prettier . --write && eslint . --fix",
"format:delta": "npm-run-all format:prettier:delta format:eslint:delta",
"format:prettier:delta": "prettier --write `./scripts/changed-files`",
"format:eslint:delta": "eslint --fix `./scripts/changed-files`",
"test": "vitest run --coverage",
"test:watch": "vitest",
"test:update": "vitest run --update",
"setup": "npm install && npm run validate",
"validate": "npm-run-all lint test",
"validate": "npm-run-all test",
"contributors:add": "all-contributors add",
"contributors:generate": "all-contributors generate"
},
Expand All @@ -72,6 +78,7 @@
"eslint-config-prettier": "9.1.0",
"eslint-config-standard": "17.1.0",
"eslint-plugin-import": "2.29.1",
"eslint-plugin-json-files": "^4.1.0",
"eslint-plugin-n": "16.6.2",
"eslint-plugin-promise": "6.1.1",
"eslint-plugin-simple-import-sort": "10.0.0",
Expand Down
3 changes: 3 additions & 0 deletions scripts/changed-files
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/usr/bin/env bash

git diff --name-only --diff-filter=d main
Loading