-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add formatter? #336
Comments
Yes, we recommend using those in the guide: https://guide.esciencecenter.nl/#/best_practices/language_guides/python?id=coding-style-conventions Given the popularity of It would be nice to add a pre-commit hook for it too #222. |
My two cents: I personally hate Btw, does it make sense to use an autoformatter while also maintaining a linter config? Or can autoformatter and linter configs be unified? Just thinking about maintainability of the template. |
Linting does a lot more than just checking formatting |
@egpbos, only |
I personally prefer To be honest, I've already started having doubts about using the template for some of my projects in the past, because it feels too bloated to me. I looked into making components optional, but that's not easy with cookiecutter. I also thought about creating a separate minimalist version, but that would just create more confusion and discussion and maintenance burden that I don't think we want. Anyway, rather than follow my philosophical tangent, let's keep the discussion practical: if you think it's useful for you, let's just add it :) People can take it out again from their projects if they prefer. At least then it's consistent with the Guide as @bouweandela pointed out. |
On "The bird project", AKA "Where The Flock", AKA "The one @lyashevska is leading", we decided to go with |
Github implicitly recommends
I have been using ruff for a while now, and I find it preferable over Black because of its speed and because it also has an |
Ah, I wasn't aware that ruff can do autoformatting as well (I see it calls it "autofix"...). In lilio it's also used for linting. Not sure whether they also use the autofix functionality. What do people think about replacing everything (i.e. linting and autoformatting) with ruff? @abelsiqueira, @lyashevska, how have you integrated |
We use Currently, our I am also considering moving to |
@egpbos Ruff's autofix is for automatically fixing linting issues. Note that not all warnings/errors have an autofix available. |
Ok, so ruff is not a replacement for autoformatting, although maybe partially? Do you have a clear view on which parts can get autofixed @BSchilperoort? I guess having config for both might be nice then. Replacing prospector with ruff sounds like a good plan @abelsiqueira, it seems like the support level for prospector is significantly lower than ruff currently. If you do that on your project, please ping us here as well, and we can try to get it integrated in the template. I think especially autoformatting makes sense only in pre-commit, not so much in CI where it is basically already too late. From @abelsiqueira's example, it seems very simple to include black, so let's just do that. Of course, it would be nice if we also add a test to the template's test suite that makes sure it works. Anyone feel up to making a PR for this? :) |
It's mostly simpler stuff. For example "none-comparison" (Comparison to None should be cond is None) has an autofix. However, rules like "line-too-long" or "mixed-spaces-and-tabs" do not have an autofix. The Ruff documentation has an overview of all rules, and if autofixes are available for them. @egpbos @abelsiqueira do consider that when moving from prospector to Ruff, you have to choose which rules to use. Ruff has implemented many from different packages. The rulesets we use can be found here and here. |
…-and-isort use ruff instead of prospector and isort (Refs #336)
Ruff now also includes auto-formatting capabilities and is as good as feature complete compared to Black: https://astral.sh/blog/the-ruff-formatter (article from October, several updated releases since then). I think that for simplicity, in the template we should just use that. It seems like people agree that it's best to add this to a pre-commit config file. Let's move that part of this issue there (#222), since I think once pre-commit is added, adding ruff formatting, black formatting and basically anything else (perhaps commented out so that people can opt-in to particular choices, see also #361) is quite trivial, it seems. |
Should there be a formatter with an appropriate configuration file?
The text was updated successfully, but these errors were encountered: