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

fix pre-commit CI #2825

Merged
merged 19 commits into from
Dec 22, 2021
Merged

fix pre-commit CI #2825

merged 19 commits into from
Dec 22, 2021

Conversation

lorenzwalthert
Copy link
Contributor

@lorenzwalthert lorenzwalthert commented Dec 21, 2021

@pat-s by excluding todo-files/ for {styler}. Maybe we can re-activate the roxygenize hook and only skip it on CI.

@pat-s
Copy link
Member

pat-s commented Dec 21, 2021

Excluding todo-files is fine, they should not block anything :) I was not aware that exclude can be used for any hook - I am now 👍

Are the multiple styler entries in the YAML intended?

Maybe we can re-activate the roxygenize hook and only skip it on CI.

Jup, sounds like a good idea.

@lorenzwalthert
Copy link
Contributor Author

lorenzwalthert commented Dec 21, 2021

Are the multiple styler entries in the YAML intended?

Yeah, I tried to see if timeouts are applicable only to individual hooks or to all hooks. Unfortunately the latter, so it does not help splitting things up.

@lorenzwalthert
Copy link
Contributor Author

Ok, now we seem to stay under the timeout (2min 36s), compromising on the files styled though. Seems we can't get tests styled as well in that time window. -.- Given that mlr3 repos are much smaller and this repo is in maintenance mode only, I think it's acceptable. Note that this also applies to local execution.

@lorenzwalthert
Copy link
Contributor Author

Just to be on the save side, let's wait until you can make main pass, then we rebase these change (and I push some styling of the tests directory that are not catched with pre-commit).

@lorenzwalthert lorenzwalthert marked this pull request as draft December 21, 2021 20:17
@lorenzwalthert lorenzwalthert marked this pull request as ready for review December 21, 2021 22:11
@lorenzwalthert lorenzwalthert marked this pull request as draft December 21, 2021 22:12
@pat-s
Copy link
Member

pat-s commented Dec 22, 2021

Given that mlr3 repos are much smaller and this repo is in maintenance mode only, I think it's acceptable. Note that this also applies to local execution.

Fully agree.

Just to be on the save side, let's wait until you can make main pass, then we rebase these change (and I push some styling of the tests directory that are not catched with pre-commit).

Main is ok for now, it's unstable anyhow since some tests fail in a non-deterministic way.
E.g. the nightly CRON finished just fine - however it might be that the next commit to main will fail again.
Things will sort itself out in the long run but there is unfortunately some constant variation among the GHA runs.
This is not a new thing and is happening since more than a year already and meanwhile I am suspecting that GHA is the main problem.

But anyhow! LGTM and thanks a lot for the help 🚀

I assume we can use styler.mlr HEAD and the fixed commit sha was just used for testing before you merged the last PR?

@pat-s
Copy link
Member

pat-s commented Dec 22, 2021

I've tidied the precommit config, re-added the roxygenize hook but skipping it during CI.

@lorenzwalthert lorenzwalthert marked this pull request as ready for review December 22, 2021 08:46
@pat-s pat-s merged commit e1380d7 into mlr-org:main Dec 22, 2021
@lorenzwalthert lorenzwalthert deleted the patch-1 branch December 24, 2021 21:20
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.

None yet

2 participants