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.

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

Add pre-commit hook #57

Merged
merged 4 commits into from
Sep 13, 2022
Merged

Conversation

mondeja
Copy link
Member

@mondeja mondeja commented Sep 6, 2022

Hi guys! βœ‹πŸΌ

I'm facing a situation where I need to use SVGLint outside of a Javascript project, so I'm using pre-commit to run linters. SVGLint does not have the file to define hooks, so added in this PR.

To use SVGLint as a pre-commit hook:

  • Install pre-commit with pip install pre-commit
  • Create a .pre-commit-config.yaml file.
  • Define the hook in the file:
      - repo: https://github.com/birjj/svglint
        rev: v2.1.1
        hooks:
          - id: svglint
  • Run pre-commit install

Note that the previous config will not work as the revision v2.1.1 is not created yet, so this chage will require a new patch or minor version change in SVGLint. To test it, replace the key repo by https://github.com/mondeja/svglint and rev by pre-commit-hook (this branch) and run it with pre-commit run --all-files. You'll probably need to initialize the CWD as a GIT repo and create an initial commit as well.

All fields are documented at Creating new hooks in pre-commit.com. The key minimum_pre_commit_version is defined because node has been added to pre-commit as a supported language in v1.5.0.

@ericcornelissen
Copy link
Contributor

Odd πŸ˜• This seems like an weird approach to have to add support for tools to pre-commit to me...

Anyway, if this is the only way to support it I'm happy to add it to the repo πŸ™‚

Before I merge this tho, one question: did you add the --ci option because of #56 or not? If yes, could you add a comment along the lines of:

entry: svglint -C # The `-C` flag is used due to https://github.com/birjj/svglint/issues/56

@mondeja
Copy link
Member Author

mondeja commented Sep 12, 2022

Before I merge this tho, one question: did you add the --ci option because of #56 or not?

Yes. I've added the comment πŸ‘πŸΌ

Odd confused This seems like an weird approach to have to add support for tools to pre-commit to me...

Can be supported with a mirror repository like https://github.com/pre-commit/mirrors-prettier but I would need to put my own user in the repo key https://github.com/mondeja/svglint-mirror or try to transfer the repository to the pre-commit organization to be https://github.com/pre-commit/svglint-mirror.

But that approach seems much more complicated as the maintainment is not hard. Feel free to ping me to if you receive a PR involving this file and I'll review it.

@ericcornelissen
Copy link
Contributor

Yes. I've added the comment πŸ‘πŸΌ

Thanks!

Can be supported with a mirror repository like pre-commit/mirrors-prettier but I would need to put my own user in the repo key https://github.com/mondeja/svglint-mirror or try to transfer the repository to the pre-commit organization to be https://github.com/pre-commit/svglint-mirror.

But that approach seems much more complicated as the maintainment is not hard. Feel free to ping me to if you receive a PR involving this file and I'll review it.

I do believe that would be the better way to manage things (from pre-commit's perspective). Unfortunately, with only 23 repos in the org, it seems that's not the preferred way + given the relative unpopularity of this package, I don't think they'd even be inclined to accept it. Also, I agree that maintaining this shouldn't be too difficult, so I'll just merge this in and create a release πŸ™‚

@ericcornelissen ericcornelissen merged commit cb2182e into simple-icons:master Sep 13, 2022
@github-actions
Copy link

πŸŽ‰ This PR is included in version 2.1.1 πŸŽ‰

The release is available on:

Your semantic-release bot πŸ“¦πŸš€

@mondeja mondeja deleted the pre-commit-hook branch September 13, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants