- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 88
Open
Labels
code qualityImprovements to code qualityImprovements to code qualityhelp wantedExtra attention is neededExtra attention is needed
Milestone
Description
- Add import sorting. Probably should just use
import/order
, but if that doesn't work for some reason we could consider https://github.com/trivago/prettier-plugin-sort-imports, which is a prettier pluginAddeslint --fix
back to pre-commit using a local hook to ensure version parity as described in Add pre-commit #637 (comment). Ensure that it is run on Typescript files as described in https://github.com/pre-commit/mirrors-eslint#using-eslint-with-pre-commitAdd https://www.npmjs.com/package/eslint-plugin-unused-imports as eslint plugin, and configure it properly to remove importsWhile we're here, we could switch to runningprettier
using a local hook as well in order to maintain version parity withpackage.json
To pick up a draggable item, press the space bar. While dragging, use the arrow keys to move the item. Press space again to drop the item in its new position, or press escape to cancel.
auscompgeek
Metadata
Metadata
Assignees
Labels
code qualityImprovements to code qualityImprovements to code qualityhelp wantedExtra attention is neededExtra attention is needed
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
[-]Sort Typescript imports using `prettier-plugin-sort-imports`[/-][+]Cleanup Typescript imports in pre-commit[/+]eslint
: make--fix
remove unused imports #1076phillco commentedon Nov 1, 2022
From the meetup: @pokey's main concern with doing this now is version skew between pre-commit's
eslint
and what's inpackage.json
, as they should match, but pre-commit.ci only bumps the version in the pre-commit configuration, not the package.json.Semi relatedly, we're also not thrilled with pre-commit.ci's lack of respect for semver. (And I find the fact that you have to pin versions in two places in the first place to be kind of sus)
Ideally https://github.com/renovatebot/renovate would be used to bump both locations together, but we're not quite ready to pull the trigger on that, because it looks like it wants to bump everything, opening a huge number of PRs. We'd like to enable automatic merging if tests pass for these types of procedural bumps (this would also be good to have in all of the other Talon repositories -- it currently feels like busywork), but things like the cheatsheet and website do not have good test coverage currently, and currently get automatically deployed, so we don't want to risk them suddenly breaking.
In the short term, we agreed that if we could add eslint to pre-commit config but tell pre-commit.ci not to bump it (and thus require that people bump both locations together manually), that would be fine for now to unblock this. This looks pretty easy to do.
That way, we get the benefits of automatic fixing on PRs (or locally), at the cost of slightly more work to bump
eslint
until we can set up something like renovate, but hopefully we runfix
way more often than we bumpeslint
.auscompgeek commentedon Nov 3, 2022
Isn't that
ci#skip
configuration to skip the hook entirely on pre-commit.ci, hence we won't get autofixing on PRs?phillco commentedon Nov 3, 2022
Oops, you're right. It doesn't look like they separate skipping fixups from skipping version bumping for a particular hook. I guess we could maybe move to bumping versions of everything manually?
pre-commit: Switch to GitHub action; add eslint fixer (#1404)
auscompgeek commentedon Aug 24, 2023
In case we're unhappy with prettier, and alphabetical sorting works for us, dprint's typescript plugin will sort imports alphabetically by default.