search

awslabs / gluonts

Probabilistic time series modeling in Python
https://ts.gluon.ai
Apache License 2.0
4.65k stars 755 forks source link

migrate style check to precommit #3111

Open Borda opened 10 months ago

Borda commented 10 months ago

Issue #, if available: #2637 partially

Description of changes: moving some style checks to common pre-commit, which can be efficiently run by the developer locally and also for PR runs only on the diff, not the full project, so the iteration shall be much faster... In addition, a bot can be installed to this repository that may send some automated fixes related to the open PR so a developer will not think about styles too much :flamingo:

BTW, it would be good to migrate also the just license so the whole workflow can be dropped

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

cc: @jaheba @kashif @lostella kind ling as it seems there are no revivers by default :chipmunk:

Please tag this pr with at least one of these labels to make our release process faster: BREAKING, new feature, bug fix, other change, dev setup

lostella commented 10 months ago

@Borda the style check workflow is pretty quick compared to tests, I think?

Borda commented 10 months ago

the style check workflow is pretty quick compared to tests, I think?

@lostella, that is true, but with pre-commit, you can run it locally and only on changed files compared to relying on validation after pushing or calling several commands as it is in the job on whole codebase

kashif commented 9 months ago

@Borda lets move to ruff instead of black? for fix and format

Borda commented 9 months ago

@Borda lets move to ruff instead of black? for fix and format

Sure, happy to do it. Just thought to make this 1:1 chance and additional improvement in next PR, what do you think @kashif?

kashif commented 9 months ago

sounds good!

Borda commented 9 months ago

@jaheba @lostella is there anything else I could do for this PR? :chipmunk:

This merge would also mean updating required checks for future PR...

lostella commented 9 months ago

@Borda I think it's fine to have pre-commit hooks, but maybe we leave the github workflows untouched for now? We can always remove them later if we believe it's the way to go. But I think having the workflow active in the repo is a clear reference on what checks the code is expected to pass.

Borda commented 9 months ago

maybe we leave the github workflows untouched for now?

Sure, we'll put them back, just if I may suggest, lest mark them as not required and make the pre-commit bot required; what do you think?

well we can also have the commit running locally so if any outage it will hold https://github.com/marketplace/actions/pre-commit

Borda commented 9 months ago

@lostella noticed that probably lints did not have a fixed version, so it could run anytime in the future with different changes... so the precommit config fixes that and guarantees reproducibility...

anyway is it fine if I run the formating and fix issues as part of this PR? #3130 and #3131

Borda commented 6 months ago

@lostella, this is one now ready for review too :flags:

Borda commented 6 months ago

@lostella friendly ping :flamingo:

Borda commented 4 months ago

@lostella friendly ping :chipmunk:

Borda commented 4 months ago

@lostella seems all is :green_circle: now :tada:

Borda commented 3 months ago

@lostella seems all is 🟢 now 🎉

mind have a look, when you have time 🦩

Borda commented 2 months ago

@lostella could you pls have a look? 🐿️

Borda commented 1 month ago

@lostella could you pls have a look? 🐿️

seems GH changed roles for CI; would you mind approving this PR or adding me there...

Borda commented 2 weeks ago

@lostella could you pls have a look? 🐿️

seems GH changed roles for CI; would you mind approving this PR or adding me there...

@lostella let me know if I should resolve the conflicts for merging...