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

consistently delegate linting concerns to pre-commit #47

Merged
merged 11 commits into from
Feb 16, 2023
Merged

consistently delegate linting concerns to pre-commit #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
99c3a5d
completely delegate prettier to pre-commit
chris48s Jan 19, 2023
12cbd2f
exclude ignored files
chris48s Jan 19, 2023
5ec316b
move eslint and stylelint to pre-commit
chris48s Jan 20, 2023
0cdf907
don't ignore js files that start with a dot (e.g: .stylelintrc.js)
chris48s Jan 20, 2023
02648fb
disable unused vars check in custom.d.ts
chris48s Jan 20, 2023
f346f29
bump prettier, note config file location
chris48s Jan 20, 2023
cfee5e7
bump all the versions
chris48s Jan 24, 2023
50beb76
move pre-commit command to docs
chris48s Jan 28, 2023
bf9a708
bump isort
chris48s Jan 28, 2023
ff8595f
docs tweak
chris48s Jan 28, 2023
724c1dd
make pre-commit a testing dependency
chris48s Feb 16, 2023
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
3 changes: 3 additions & 0 deletions {{ cookiecutter.__project_name_kebab }}/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@ module.exports = {
},
},
},
// ESlint default behaviour ignores file/folders starting with "."
// https://github.com/eslint/eslint/issues/10341
ignorePatterns: ['!.*', 'node_modules', 'dist'],
};
36 changes: 31 additions & 5 deletions {{ cookiecutter.__project_name_kebab }}/.pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ default_language_version:

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.3.0
rev: v4.4.0
hooks:
- id: check-added-large-files
- id: check-case-conflict
Expand All @@ -26,23 +26,49 @@ repos:
args: ['--target-version', 'py37']
- repo: https://github.com/pycqa/isort
# isort config is in setup.cfg
rev: 5.10.1
rev: 5.12.0
hooks:
- id: isort
- repo: https://github.com/pycqa/flake8
# flake8 config is in setup.cfg
rev: 5.0.4
rev: 6.0.0
hooks:
- id: flake8
additional_dependencies:
- flake8-bugbear
- flake8-comprehensions
- repo: https://github.com/pre-commit/mirrors-prettier
# note: keep in sync with the version from package.json
rev: 'v1.18.2'
# prettier config is in prettier.config.js
rev: 'v2.7.1'
hooks:
- id: prettier
types_or: [css, scss, javascript, ts, tsx, json, yaml]
- repo: https://github.com/pre-commit/mirrors-eslint
# eslint config is in .eslintrc.js
rev: v8.32.0
hooks:
- id: eslint
additional_dependencies:
- '[email protected]'
- '[email protected]'
- '[email protected]'
- '[email protected]'
- '[email protected]'
- '[email protected]'
- '@typescript-eslint/[email protected]'
- '@typescript-eslint/[email protected]'
- '@wagtail/[email protected]'
Comment on lines +51 to +60
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is a blocker, but another pre-commit question as I continue my odyssey of trying to get on-board with it:
It seems like pre-commit requires the user to resolve their own dependency tree here. Whereas with NPM we can just specify the top-level plugins we care about (@typescript-eslint/eslint-plugin, @typescript-eslint/parser, @wagtail/eslint-config-wagtail) and leave NPM to resolve the transitive dependencies, that doesn't work with pre-commit. It seems you need to explicitly specify each plugin/config referenced in eslintrc in .pre-commit-config.yaml (this is what we do in WTKit too for reference). I was able to work it out by npm install-ing everything into a clean project somewhere else, letting NPM resolve the constraints and then using npm list to extract the packages/version numbers from the resolved tree to manually plug back in to the yaml file, but this doesn't feel like an ideal workflow to impose on package authors as they maintain/update this over time. It is not something I would ideally choose to live with on an ongoing basis. Am I doing it wrong? Is there a better solution for this that exists in pre-commit land?. This seems to be a characteristic which is mostly ESLint-specific as a typical ESLint config relies on many third-party plugins and configs, which is not the case for most other tools we use.

files: \.(js|jsx|ts|tsx)$
types: [file]
- repo: https://github.com/awebdeveloper/pre-commit-stylelint
# stylelint config is in .stylelintrc.js
rev: 8f63da497580898a7e0ceef6bf9e72cc0af07828
hooks:
- id: stylelint
files: \.(scss)$
additional_dependencies:
- '[email protected]'
- '@wagtail/[email protected]'
- repo: https://github.com/asottile/blacken-docs
rev: v1.12.1
hooks:
Expand Down
7 changes: 3 additions & 4 deletions {{ cookiecutter.__project_name_kebab }}/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,18 +64,17 @@ flit install

### pre-commit

Note that this project uses [pre-commit](https://github.com/pre-commit/pre-commit). To set up locally:
Note that this project uses [pre-commit](https://github.com/pre-commit/pre-commit).
It is included in the project testing requirements. To set up locally:

```shell
# if you don't have it yet, globally
$ python -m pip install pre-commit
# go to the project directory
$ cd {{ cookiecutter.__project_name_kebab }}
# initialize pre-commit
$ pre-commit install

# Optional, run all checks once for this, then the checks will run only on the changed files
$ pre-commit run --all-files
$ git ls-files --others --cached --exclude-standard | xargs pre-commit run --files
```

### How to run tests
Expand Down
14 changes: 1 addition & 13 deletions {{ cookiecutter.__project_name_kebab }}/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,7 @@
"main": "{{ cookiecutter.__project_name_snake }}/static_src/main.tsx",
"scripts": {
"start": "webpack --config ./webpack.config.js --mode development --progress --watch",
"build": "webpack --config ./webpack.config.js --mode production",
"format": "prettier --write '**/?(.)*.{css,scss,js,ts,tsx,json,yaml,yml}'",
"lint:js": "eslint --ext .js,.ts,.tsx --report-unused-disable-directives --max-warnings 16 ./",
"lint:css": "stylelint **/*.scss",
"lint:format": "prettier --check '**/?(.)*.{css,scss,js,ts,tsx,json,yaml,yml}'",
"lint": "npm run lint:js && npm run lint:css && npm run lint:format"
"build": "webpack --config ./webpack.config.js --mode production"
},
"author": "",
"license": "{% if cookiecutter.open_source_license == 'MIT license' -%}MIT{% elif cookiecutter.open_source_license == 'BSD license' %}BSD{% elif cookiecutter.open_source_license == 'ISC license' -%}ISC{% elif cookiecutter.open_source_license == 'Apache Software License 2.0' -%}Apache{% elif cookiecutter.open_source_license == 'GNU General Public License v3' -%}{% endif %}",
Expand All @@ -19,18 +14,11 @@
"@types/react": "^16.8.19",
"@types/react-dom": "^16.8.19",
"@types/styled-components": "^5.1.23",
"@typescript-eslint/eslint-plugin": "^5.12.1",
"@typescript-eslint/parser": "^5.12.1",
"@wagtail/eslint-config-wagtail": "^0.4.0",
"@wagtail/stylelint-config-wagtail": "^0.5.0",
"eslint": "^8.9.0",
"file-loader": "^6.2.0",
"postcss-loader": "^6.2.1",
"postcss": "^8.4.5",
"prettier": "^2.5.1",
"sass-loader": "^12.4.0",
"sass": "^1.45.3",
"stylelint": "^14.5.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

love a good dependency tidy up! (granted this has shifted to pre-commit, but still)

"ts-loader": "^9.2.6",
"typescript": "^4.5.5",
"webpack": "^5.65.0",
Expand Down
3 changes: 2 additions & 1 deletion {{ cookiecutter.__project_name_kebab }}/pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ dependencies = [
]
[project.optional-dependencies]
testing = [
"dj-database-url==1.2.0"
"dj-database-url==1.2.0",
"pre-commit>=2.21.0,<3.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pinned to pre-commit 2.x because pre-commit 3 drops compatibility with python 3.7 and this project still includes 3.7 in the test matrix/classifiers. Once we drop it, we can bump this to pre-commit 3

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏼 #50

]

[project.urls]
Expand Down
1 change: 1 addition & 0 deletions ...er.__project_name_kebab }}/{{ cookiecutter.__project_name_snake }}/static_src/custom.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable no-unused-vars */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this here because if I just make a new package and run eslint on the output, I get

  30:9   error  'wagtailConfig' is defined but never used  no-unused-vars
  35:12  error  'gettext' is defined but never used        no-unused-vars
  35:20  error  'text' is defined but never used           no-unused-vars
  38:12  error  'ngettext' is defined but never used       no-unused-vars
  38:21  error  'singular' is defined but never used       no-unused-vars
  38:39  error  'plural' is defined but never used         no-unused-vars
  38:55  error  'count' is defined but never used          no-unused-vars
  60:12  error  'get_format' is defined but never used     no-unused-vars
  60:23  error  'formatType' is defined but never used     no-unused-vars
  63:12  error  'gettext_noop' is defined but never used   no-unused-vars
  63:25  error  'text' is defined but never used           no-unused-vars
  66:12  error  'pgettext' is defined but never used       no-unused-vars
  66:21  error  'context' is defined but never used        no-unused-vars
  66:38  error  'text' is defined but never used           no-unused-vars
  69:21  error  'context' is defined but never used        no-unused-vars
  69:38  error  'text' is defined but never used           no-unused-vars
  69:52  error  'count' is defined but never used          no-unused-vars
  72:12  error  'pluralidx' is defined but never used      no-unused-vars
  72:22  error  'count' is defined but never used          no-unused-vars

✖ 19 problems (19 errors, 0 warnings)

This is not unexpected as none of these type definitions are used by any code that exists (yet). I think if we want to be able to run eslint on this and have it pass, we do need to silence that error somehow.

I am slightly unclear whether we want to render the package output with that check disabled when a user generates a new package. I think I need to understand slightly more about the rationale for including this specific selection of type definitions in the first place to make the right decision here.

export {};

// Allows SVG files to be imported and used in TypeScript
Expand Down