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-hooks.yaml #872

Merged
merged 10 commits into from Mar 15, 2021
Merged

Add .pre-commit-hooks.yaml #872

merged 10 commits into from Mar 15, 2021

Conversation

@rkm
Copy link
Contributor

@rkm rkm commented Nov 21, 2020

Hi! pre-commit now supports the dotnet language natively, which is stable as of v2.9.0. This PR adds a .pre-commit-hooks.yaml file so that dotnet format can be easily integrated into the workflows of developers who use pre-commit.

A sample .pre-commit-config.yaml to use this hook would then look like:

repos:
-   repo: https://github.com/dotnet/format
    rev: "v4.0.130203"  # Or a more recent commit sha
    hooks:
    -   id: dotnet-format
        args: [--include]

I'm not sure how best to document this. Many maintainers add a section in their project README which provides a sample hook like the one above. I see from #692 that there are already some notes in the docs/integrations.md file, which I could add this to instead?

Closes #692

@chriselion
Copy link
Contributor

@chriselion chriselion commented Jan 24, 2021

Do you need to worry about #699 for large projects?

I added pre-commit support for my team's repo a few months ago (before pre-commit supported dotnet natively), and wrote the files names to a temp file before passing to dotnet-format just to be on the safe side.

@jmarolf
Copy link
Member

@jmarolf jmarolf commented Jan 25, 2021

Thanks for this @rkm! In order to merge this we will need to update the integrations.md file to talk about how you set this up. I'll review the pre-commit hook format so I can review this as best I can.

NOTE: dotnet-format supports both C# and Visual Basic so I think the language parameter should reflect that but I will need to do more research.

@chriselion
Copy link
Contributor

@chriselion chriselion commented Jan 25, 2021

The language field tells pre-commit what language to execute in, so I think that needs to stay as it is. But if you wanted to support Visual Basic file extensions, you could change the types to types_or and add the extension there.

Base automatically changed from master to main Mar 5, 2021
@chriselion
Copy link
Contributor

@chriselion chriselion commented Mar 12, 2021

@jmarolf Sorry for the bump, but any update on this?

@jmarolf
Copy link
Member

@jmarolf jmarolf commented Mar 12, 2021

To merge this we need integrations.md to be updated to describe how someone consumed pre-commit hooks

@chriselion
Copy link
Contributor

@chriselion chriselion commented Mar 12, 2021

Ah sorry, that was a misunderstanding on my part; I interpreted

In order to merge this we will need to update the integrations.md file to talk about how you set this up

to mean that you or the other maintainers would update integrations.md. I'll see if I can push to this branch (or make another PR against it)

@dnfadmin
Copy link

@dnfadmin dnfadmin commented Mar 14, 2021

CLA assistant check
All CLA requirements met.

@chriselion
Copy link
Contributor

@chriselion chriselion commented Mar 14, 2021

@jmarolf - I updated integrations.md, and also changed the file types so that Visual Basic files will be checked too.

Copy link
Member

@JoeRobich JoeRobich left a comment

Thanks for contributing this!

@JoeRobich JoeRobich merged commit 7e34307 into dotnet:main Mar 15, 2021
18 checks passed
@rkm rkm deleted the feature/pre-commit branch Mar 15, 2021
@chriselion
Copy link
Contributor

@chriselion chriselion commented Mar 15, 2021

Thanks @JoeRobich! Any chance you'll be doing a release that includes this change anytime soon? I can update the integrations.md with a specific tag once it exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants