migrate style check to precommit #3111
Conversation
|
@Borda 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 |
|
@Borda lets move to ruff instead of black? for fix and format |
|
sounds good! |
|
@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. |
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 |
*Issue #, if available:* set a single black version to ensure reproducibility UPDATE: seem that the latest Black would need to be applied *Description of changes:* freeze Black version, but the better way is in #3111 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. **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 cc: @jaheba @kashif @lostella
*Issue #, if available:* `dofformatter` gave a false positive signal; it has lots of issues, but as `--check` was missing, it just fixed without any real code change and so passed green even if it failed, see for example https://github.com/awslabs/gluonts/actions/runs/7975893111/job/21775154024?pr=3130 *Description of changes:* validation of the actual Ruff & Docfroatter on the latest codebase **Please tag this pr with at least one of these labels to make our release process faster:** this is just fixing the actual linting issue, but the better and ext step shall be #3111 cc: @jaheba @kashif @lostella --------- Co-authored-by: Lorenzo Stella <[email protected]>
|
@lostella, this is one now ready for review too 🎏 |
|
@lostella friendly ping 🦩 |
*Issue #, if available:* set a single black version to ensure reproducibility UPDATE: seem that the latest Black would need to be applied *Description of changes:* freeze Black version, but the better way is in awslabs#3111 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. **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 cc: @jaheba @kashif @lostella
*Issue #, if available:* `dofformatter` gave a false positive signal; it has lots of issues, but as `--check` was missing, it just fixed without any real code change and so passed green even if it failed, see for example https://github.com/awslabs/gluonts/actions/runs/7975893111/job/21775154024?pr=3130 *Description of changes:* validation of the actual Ruff & Docfroatter on the latest codebase **Please tag this pr with at least one of these labels to make our release process faster:** this is just fixing the actual linting issue, but the better and ext step shall be awslabs#3111 cc: @jaheba @kashif @lostella --------- Co-authored-by: Lorenzo Stella <[email protected]>
|
@lostella friendly ping 🐿️ |
|
@lostella seems all is 🟢 now 🎉 |
mind have a look, when you have time 🦩 |
|
@lostella could you pls have a look? 🐿️ |
seems GH changed roles for CI; would you mind approving this PR or adding me there... |
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 🦩
BTW, it would be good to migrate also the
just licenseso the whole workflow can be droppedBy 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 🐿️
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