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

Use CI pre-commit action. #120

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Use CI pre-commit action. #120

merged 5 commits into from
Feb 9, 2024

Conversation

YooSunYoung
Copy link
Member

@YooSunYoung YooSunYoung commented Dec 15, 2023

Fixes #104
I updated the ci.yml of the copier_template workflow to use pre-commit action itself.
The template is not updated yet.

TODO:

  • Install pre-commit action in repositories (including copier_template)
  • Add pre-commit action in template ci recipes

It was tested with essnmx: scipp/essnmx#12.

@SimonHeybrock
Copy link
Member

What is the status here? @jl-wynen, you opened #104 so maybe you are best as a reviewer.

@YooSunYoung YooSunYoung marked this pull request as ready for review February 8, 2024 13:48
@YooSunYoung
Copy link
Member Author

@SimonHeybrock
Just need to check if Install pre-commit action in repositories (including copier_template) is done...!

@jl-wynen
Copy link
Member

jl-wynen commented Feb 9, 2024

What is the status here? @jl-wynen, you opened #104 so maybe you are best as a reviewer

What @YooSunYoung said. As far as I'm concerned, we should switch to this action. I just installed the application in this repo. I'm happy to do it for all repos but I didn't want to do it without an explicit 'go ahead'.

- run: python -m pip install -r requirements/ci.txt
- run: tox -e static
- uses: stefanzweifel/git-auto-commit-action@v5
- uses: pre-commit/[email protected]
Copy link
Member

Choose a reason for hiding this comment

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

Not the latest version. Should we actually pin down to the patch version? We don't do this elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by pin down...?
And yeah, I see they 3.0.1 is released.

Copy link
Member

Choose a reason for hiding this comment

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

I meant 'down to' as a preposition. I.e., pin all parts of the version number. I think we often only pin the major and maybe minor version. But not the patch version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh like that...! Okay, then I'll get rid of .0.0

Copy link
Member

Choose a reason for hiding this comment

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

Apparently this doesn't work. In that case, I'm fine with using the full version

Copy link
Member Author

Choose a reason for hiding this comment

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

@jl-wynen Apparently pre-commit ci action doesn't work like that...

Copy link
Member Author

Choose a reason for hiding this comment

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

@YooSunYoung YooSunYoung merged commit 1abe96f into main Feb 9, 2024
2 checks passed
@YooSunYoung YooSunYoung deleted the ci-pre-commit branch February 9, 2024 09:39
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.

Consider pre-commit-ci/lite
3 participants