kraken-build / kraken

The Kraken build system.
https://kraken-build.github.io/kraken/
28 stars 12 forks source link

Add `RuffTask` and `ruff()` to allow using Ruff as python formatter and linter #255

Closed Ghelfi closed 4 months ago

Ghelfi commented 5 months ago

Description

The goal of this PR is to add Ruff as a new formatter / linter. Fours tasks are added:

Ghelfi commented 5 months ago

For now, only RuffTask are defined but not called within the project.

Do we want to use ruff as linter in the repo itself ?

NiklasRosenstein commented 5 months ago

@Ghelfi FYI the CI runners are a bit.. unstable. So there may be some flakes unrelated to your changes, usually reruns fix them.

NiklasRosenstein commented 5 months ago

Very nice @Ghelfi 👏 A few points/questions before we proceed with the merge:

Ghelfi commented 5 months ago

Very nice @Ghelfi 👏 A few points/questions before we proceed with the merge:

  • [ ] It would be great to have a simple unit test that sets up the Ruff tasks, executes them and checks the results (e.g. did Ruff actually modify the file as expected to fix/reformat or raise any lints?)

    • You can very easily just depend on the kraken_project fixture in your test and use it's .directory property (a temporary directory) to copy a Python file into it and then run the Ruff tasks with kraken_project.context.execute()
  • [ ] Do we need to worry about options to tell Ruff to include/exclude certain paths, or is it generally clever enough to be doing the right thing? (E.g. skip the contents of a .venv directory)

Sure for the first point, I'll have a look. For the second point I think we are good. I made a test with the following setup:

tmp_dir/
├── tmp.py
├── .venv/
│   └── tmp.py
├── build/
│   └── tmp.py
└── custom_package/
    └── tmp.py

with tmp.py a file to be formatted. Ruff was called from within tmp_dir and formatting was applied only on the root level file and the one in custom_package.

Ghelfi commented 5 months ago

Very nice @Ghelfi 👏 A few points/questions before we proceed with the merge:

  • [ ] It would be great to have a simple unit test that sets up the Ruff tasks, executes them and checks the results (e.g. did Ruff actually modify the file as expected to fix/reformat or raise any lints?)

    • You can very easily just depend on the kraken_project fixture in your test and use it's .directory property (a temporary directory) to copy a Python file into it and then run the Ruff tasks with kraken_project.context.execute()
  • [ ] Do we need to worry about options to tell Ruff to include/exclude certain paths, or is it generally clever enough to be doing the right thing? (E.g. skip the contents of a .venv directory)

Tests implemented. There were no unit tests for the tasks. I put the new ones in the test directory even if some exists alongside the implementation in src. @NiklasRosenstein tell me if you want me to move the tests somewhere else. I am checking that the fmt and lint operates as expected individually but also when paired together.

NiklasRosenstein commented 4 months ago

Looks 👌 to me as well, good to go once CI passed. 🚀

Thanks @Ghelfi

NiklasRosenstein commented 4 months ago

I need to dig a bit into why CI is now failing, seems like some change to Docker behaviour that we've been implicitly relying upon.