freqtrade
freqtrade copied to clipboard
freqtrade
use black as formatter
What's your stance on using black as formatter for the project?
It doesn't respect a couple of flake8 rules,
ignore = F403, F405, W503, E203, E231
And the thing that might annoy someone is that if a sequence of tokens doesn't fit inlineit gets fully split vertically
I remember we've been discussing it with @xmatthias , I do not have any objections against it.
i do like the idea of black beeing a "no compromise" formatter - as we can just run it and there's no discussion as to "let's format this that way".
I am however sceptical on introducing it "in one rush" in a project this size.
I did try to use it for freqtrade about a year ago - but it does a large amount of changes to the simplest files i wasn't comfortable it wouln't accidentally introduce bugs (it'll be impossible to review as every other line is changed...).
Now that said - i think black has matured (it was pretty fresh back then) - so i think the probability of it introducing bugs is pretty low - so i'm not against it.
There is another caveat though - i'd like to keep the 100 character line limit (80 is too small in my opinion) - and to configure black for that, we MUST use a pyproject.toml file. The problem with adding this file is that pip tends to freak out if it's present but not the method to install (or at least it was a few pip versions ago)- so i think we'll need to change the installation method to pyproject.toml first (*unless pip now handles this correctly).
"i'd like to keep the 100 character line limit" -- sure, this should be kept in any case, by configuring the formatter accordingle when (and if) one will be used.
Additional point - if we're to use black - we should also consider using pre-commit - it can be configured to run black right before commiting - which should avoid commits like "make flake happy" - or "fix flake" ...
Additional point - if we're to use black - we should also consider using pre-commit - it can be configured to run black right before commiting - which should avoid commits like "make flake happy" - or "fix flake" ...
no problem for me...
tbh, i don't see any rush with this - and think that "right after it's merged" is a bad idea. doing a "black" step will change many files, making the tracking of changes difficult as many lines will have "black" against it's changes.
Maybe 1-2 months after that.
Does github natively support it? no. Therefore - give it 1-2 months minumum, then i'll look into this (i'll not accept PR's for this - but will do this change myself once the time is right as reviewing such a change would take more time than doing the actual change).
Sounds like the managing of technical debt is a bit in the way of moving this forward? How about reviewing sections of the codebase and getting them to be as Black wants it to be, instead of using a commit hook? Personally I'm not too fond of pre-commit hooks that modify files underneath my feet 😬 Also, implementing other linters such as https://github.com/charliermarsh/ruff might make it easier to manage? Or an overview in a static analysis tool such as Sonar?
Don't see how this comment is related to this issue, really. everyone is free to not install pre-commit hooks if they don't like them. Which hooks run is a different topic entirely. We run the same checks in CI - so if the code in a PR doesn't correspond to the guidelines currently applied, CI will fail - which implicitly asks for a change. If you didn't run the pre-commit hook - it'll be an additional roundtrip - but it's not really relevant either.
changing to black will change every python file in the project. I've got a huge change in a parallel PR (not yet a PR) which is about 80-90% ready (but it's kinda stuck as i'm tight on time). I'm however not ready to invest the time twice - once to update to black (ensuring it'll break nothing) - and then trying to merge that on top of that pr PR - which is changing at least half of the python files in the repository.
There's no rush to it - and nobody's profits will improve by having a different code style applied - therefore i also treat this as very low priority.