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

Create a pre-commit-hook for sqruff #742

Open
concur1 opened this issue Sep 13, 2024 · 4 comments
Open

Create a pre-commit-hook for sqruff #742

concur1 opened this issue Sep 13, 2024 · 4 comments

Comments

@concur1
Copy link
Contributor

concur1 commented Sep 13, 2024

Pre commit hooks can be a useful development workflow that allow formatters/linters to run before a change is commited to git. There is a python pre-commit-hook utility that allows these to be configured with yaml. Here is an example of Ruff's pre-commit-hook: https://github.com/astral-sh/ruff-pre-commit

@concur1
Copy link
Contributor Author

concur1 commented Oct 4, 2024

Hi @benfdking and @gvozdvmozgu I have created a pre-commit hook here: https://github.com/concur1/sqruff-pre-commit

I mainly tried to just copy the ruff pre-commit hook, but based it on cargo/rust instead of python/pypi as I thought that is more appropriate here.

I was wondering if you would want to take ownership of this or if you would prefer to leave it with me?

It works (from my testing at least) but there is one small thing that someone who knows more about rust might be able to help with: I have been forced to include a dummy/useless src/main.rs file even though it does not get used. As far as I can tell cargo requires some src files to exist. Does anyone know if there is a cargo.toml config that can be used to allow building with no source files?

@gvozdvmozgu
Copy link
Collaborator

Why do we need to create a package?

@benfdking
Copy link
Collaborator

I presume we can just take the commit yaml file and merge that into this repo?

@concur1
Copy link
Contributor Author

concur1 commented Oct 4, 2024

Why do we need to create a package?

When people run pre-commit it will point to this repo and install sqruff for them based on the cargo.toml. I will give reasons why I think it needs to be a separate repo (for now) bellow.

I presume we can just take the commit yaml file and merge that into this repo?

I don't think that is possible with the current sqruff structure, we would need the root cargo.toml to create a sqruff binary, and ideally that would be all that it does. I will explain more below.

I couldn't find any simimlar rust examples. From reading the pre-commit documentation and asking in their discord it seems that:

  1. The pre-commit hook documentation specifies that the hook must exist in a repo with "a Cargo.toml file which produces at least one binary (example), whose name should match the entry definition for your hook.". - in this case it is the sqruff binary.
  2. Both the .pre-commit-hook.yaml and the cargo.toml must live in the repo root.
  3. Currently the cargo.toml that produces the sqruff binary is under crates/cli so it will not get picked up by pre-commit.
  4. There doesn't seem to be a way of passing a path to the desired cargo.toml (not found in the docs and I have asked in discord).

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

No branches or pull requests

3 participants