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

feat: added support for pre-commit #1396

Merged
merged 3 commits into from
Apr 15, 2024
Merged

Conversation

MattTimms
Copy link
Contributor

Adds support for pre-commit as per discussion #1360

Relies on the docker image releases, & this referenced image tag shouble be incremented with each package release; i.e. updating the entry field in .pre-commit-hooks.yaml.

An example .pre-commit-config.yaml is provided in the changes to the README.

@knightdave
Copy link

knightdave commented Apr 3, 2024

Hello,
I just was about to create new fork and implement pre-commit hook for my beloved lychee, but I saw this PR.

Do you think it would be beneficial to add 2 types of hooks - system and docker based like in hadolint hooks or kube-linter hooks?

I'm about to implement pre-commit for documentation link checking for quite big project with hundreds of developers and some of environments where they are working don't have access to install docker engine, but they could download binary and put in local user path.

Please consider this in your PR!

Copy link

@knightdave knightdave Apr 3, 2024

Choose a reason for hiding this comment

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

---
- id: lychee-docker
  name: lychee
  description: "Run 'lychee' docker image for fast, async, stream-based link checking"
  entry: lycheeverse/lychee:0.14
  language: docker_image
  args: []
  types: [text]
  pass_filenames: true
  require_serial: true
  additional_dependencies: []
- id: lychee
  name: lychee
  description: "Run 'lychee' for fast, async, stream-based link checking"
  entry: lychee
  language: system
  args: []
  types: [text]
  pass_filenames: true
  require_serial: true

Choose a reason for hiding this comment

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

I rechecked commit and it looks like it could be good to add --no-progress or -n as default argument. Without that it sometimes wait for enter or

image

Without -n
image

With -n
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find. I've had a quick look over the other args from --help & seems like --no-progress is the only that makes sense being a default.

@MattTimms
Copy link
Contributor Author

MattTimms commented Apr 3, 2024

Yeah, great suggestion @knightdave & I agree. When I was looking into originally, I was focused on getting it running from source rather than a system executable from release, which I reckon the latter is more common. I do like the idea of not requiring docker too.

@MattTimms MattTimms requested a review from knightdave April 3, 2024 22:58
@MattTimms MattTimms requested a review from knightdave April 4, 2024 22:58
Copy link

@knightdave knightdave left a comment

Choose a reason for hiding this comment

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

Looks perfect! Thank you for adding all suggestions.

@mre
Copy link
Member

mre commented Apr 15, 2024

Thanks for your contribution @MattTimms and for the insights @knightdave. Great team effort!

@mre mre merged commit eb12064 into lycheeverse:master Apr 15, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants