search

VSEScala / Scala-Dining

Contains the Scala Dining Website Design
4 stars 5 forks source link

Better formatting #256

Closed LenaWil closed 1 year ago

LenaWil commented 1 year ago

Copy some settings to pyproject, because they differed.

Make black less strict. It still shouldn’t be run and I agree with the comment earlier, but now it messes up less if I accidently run it.

import-order-style = pycharm in flake was being ignored, because the correct dependency wasn’t installed.

Swap W503 for W504 in flake8, since some new tools do that and flake8 complained about both (although that might just something on my side) making it impossible to split long binary equations and stay withing the line limit.

Added the auto formatting tools isort & autopep8 to dev-dependencies, because flake8 doesn’t auto fix them.

LenaWil commented 1 year ago

I think I encountered a bug in isort: https://github.com/PyCQA/isort/issues/2131.

mhvis commented 1 year ago

The linting now fails because our imports do not follow the pycharm import order. Maybe we should remove import-order-style = pycharm instead.

I see that backports-zoneinfo is removed in requirements.txt. It needs to stay for people that have a lower Python version than 3.10. I don't know how we can force pip-compile to leave that dependency intact. Can you add it back manually for now?

LenaWil commented 1 year ago

Maybe we should remove import-order-style = pycharm instead.

Well, you are the one who added it here, so if you’re fine with it…

mhvis commented 1 year ago

Yep I'm fine with removing it.

We could apply the Black formatter over the whole code. Then we can replace flake8 as linter. I've never tried Black. I don't know if it would be an improvement or advantage, but I'm willing to try it. @DutcherNL needs to be on board with that.

LenaWil commented 1 year ago

I am fine with that but flake8 does have some extra non formatting stuff black doesn’t have, e.g. unused variable’s and it is possible to have both at the same time, so maybe keep them both? They do work together, black is just stricter and less configurable.

I do also really prefer doing it in one commit, at the Knights I couldn’t get the others to agree to convert the whole code base in one go and we just do it when we edit the files instead, which does kinda mess up the diffs and the GitHub action messages at Squire might be too verbose at the moment but fixing it takes time. And, doing it in one commit means we can add stuff like precommit to the repo and have it automatically fix peoples commits.
(Yes, I know that how it’s done at the Knights at the moment is far from perfect but we didn’t have a linter at all before so I was happy with any improvement.)

@erictrl did say he didn’t agree with all choices black made, mostly all the vertical spacing it added, but he did see advantages of the strictness too.

Also I just haven’t gotten around removing the line because of time issues. (Typing this on mobile) Maybe in the weekend.

mhvis commented 1 year ago

I agree that a format change should happen in 1 huge PR/commit. We need to avoid commits with both format and functionality changes mixed together, because then the diffs become unreadable. And we could end up with mixed styles throughout the code.

I don't mind which style we use as long as we have a consistent style everywhere. Which is the whole point of Black. So I would be fine with whatever choices it makes. But on the other hand Flake8 already works pretty well albeit less strict.