Skip to content

Conversation

@flagarde
Copy link
Collaborator

This PR add pre-commit to run clang-format

Issues: #182

@staxfur staxfur self-assigned this Aug 22, 2022
@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

We should still check if clang-format was applied using a CI task. Can you add the pre-commit action? @flagarde

@staxfur staxfur self-requested a review August 22, 2022 14:19
Copy link
Collaborator

@staxfur staxfur left a comment

Choose a reason for hiding this comment

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

The linting must be checked by the CI.

@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

In other projects I'm using this https://github.com/github/super-linter. If pre-commit applies more than just clang-format, we might want to add a pre-commit specific CI task instead.

@flagarde
Copy link
Collaborator Author

You can ask https://pre-commit.ci/ to run pre-commit on PR et push... I t should even resubmit the change back on the PR

@flagarde
Copy link
Collaborator Author

@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

Does pre-commit have any benefits over just applying clang-format? Super linter by github also tests for git leaks and other things additionally to linting the source code.

@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

Oh, super linter does the same as well. Is the config necessary? Will add a workflow quickly

@flagarde
Copy link
Collaborator Author

Yes pre-commit can run many tools on CI and on computer https://github.com/yaodaq/YAODAQ/blob/main/.pre-commit-config.yaml etc .. You can even create yours

@flagarde
Copy link
Collaborator Author

you have even this if you want :) https://github.com/oxsecurity/megalinter with pre-commit I think

@flagarde
Copy link
Collaborator Author

Normally you even don't need the CI if the tools are installed inside the https://pre-commit.ci/ . You just have to give access to the app to the repo.. There is also auto PR for upgrade of the hooks. You need only a CI if the tools can't be installed by the hooks or are not present in pre-commit.ci

@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

@certik hey, can you approve the request of adding pre-commit to the org? Until then we just use the CI task.

@staxfur staxfur requested a review from certik August 22, 2022 14:48
@staxfur staxfur added the enhancement New feature or request label Aug 22, 2022
@staxfur staxfur added this to the V2.0.0 milestone Aug 22, 2022
@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

Automatically applying configuration can break the local branch. I wouldn't recommend that. Also feel free to add more hooks, seems pretty usefull.

@flagarde
Copy link
Collaborator Author

flagarde commented Aug 22, 2022

It doesn't change automatically but it proposes PR for example : yaodaq/YAODAQ#21

@staxfur
Copy link
Collaborator

staxfur commented Aug 22, 2022

Yeah, like dependabot.

@staxfur
Copy link
Collaborator

staxfur commented Aug 23, 2022

Sadly I don't have high enough permissions on the organization to set up the pre-commit bot. So I'll merge this for now and look into reaching out to certik.

@staxfur staxfur merged commit 57c8bf6 into jupyter-xeus:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants