Skip to content

Cleanup Typescript imports in pre-commit #640

@pokey

Description

@pokey
Member

Activity

changed the title [-]Sort Typescript imports using `prettier-plugin-sort-imports`[/-] [+]Cleanup Typescript imports in pre-commit[/+] on Apr 20, 2022
phillco

phillco commented on Nov 1, 2022

@phillco
Member

From the meetup: @pokey's main concern with doing this now is version skew between pre-commit's eslint and what's in package.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 run fix way more often than we bump eslint.

auscompgeek

auscompgeek commented on Nov 3, 2022

@auscompgeek
Member

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

Isn't that ci#skip configuration to skip the hook entirely on pre-commit.ci, hence we won't get autofixing on PRs?

phillco

phillco commented on Nov 3, 2022

@phillco
Member

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?

added this to the Short list milestone on Apr 9, 2023
modified the milestones: Short list, Long list on Jul 6, 2023
auscompgeek

auscompgeek commented on Aug 24, 2023

@auscompgeek
Member

In case we're unhappy with prettier, and alphabetical sorting works for us, dprint's typescript plugin will sort imports alphabetically by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    code qualityImprovements to code qualityhelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @auscompgeek@phillco@pokey

        Issue actions

          Cleanup Typescript imports in pre-commit · Issue #640 · cursorless-dev/cursorless