Add configurations and support for auto formatting tools #4654
Conversation
|
Nice!!! Thank you for getting this set up. It's clear that the future is in automated formatting, and carefully handcrafted formatting is best left to the dustbin of history. As at least a way to get started (and avoid bikeshedding too much), maybe a good initial point would be to try to match the flake8 configuration we're already using to check style? It needn't be an exact match, but we can minimize the footprint of the initial wave of changes if we make them reasonably close. That would mean starting with |
|
I'll go through the bits of the flake configuration one at a time. By default the line length in black is 88 which they believe to be the best for shorter files apparently but that is a judgement call to some degree. Since the aim of black is to reduce line diffs, there might be something to that. It can be configured to be any number of characters fairly easily. However if there's going to be a wave of changes, I think it would be better to do it all at once. Since the goal is to minimise diffs going into the future, once you choose a line length, it should be stuck to. Trying to increase the line length in the future is likely to cause a very large number of diffs so people would have to resolve those conflicts twice instead of once. The Google style docstrings is something that black takes over, that's not really an option. Black follows Ford's famous line: 'Any colour so long as it's black.' The idea is that there's one formatting specification that isn't messed with. For this, E121, E123, E126, E241, E305, E503, and E504 (which are all currently ignored under the flake8 configuration) would be formatted automatically and consistently. There isn't a way around this really; if you'd like these to continue to be up to the developer's preference, that kind of defeats the purpose of this tool. Black's idea is to remove preference, so if you'd like to keep those idiosyncrasies I'll have to find another tool. The other flake8 ignored errors seem to be linter errors, which black doesn't do so no problems there. There's also a large number of files that are ignored by flake8. That can actually be done but it is ill-advised. Using tools such as pre-commit means that the formatters are likely to be applied to every file at some point, and having some files that are exempt from a seemingly universal formatter would be only half getting into the bath. Unless the exclusions are for the linter aspects of flake8, I don't think they should apply to black and isort, but it's not my project so up to you. The part of black's documentation that I think you're referring to with |
|
OK, you make a strong point about the advantages of the "change as much as possible all at once" approach! Better that than a slow progression of large-diff changes. I totally agree that we should avoid doing lots of fiddly configuration changes in Black, and that we shouldn't ignore files. (The main reason that some files are ignored now for flake8 is because of comment style, which needs a lot of intervention—and which, to be honest, hasn't been all that fun to enforce manually.) (And sorry for misreading the docs! I totally see now I was getting the I obviously think it's best to avoid bikeshedding style stuff as much as possible, but I can't help suggesting that we stick to 79 or 80 columns for the default. I may just be old, but I often find tall & skinny code a lot easier to navigate and edit; 80 columns is really my sweet spot for fitting multiple windows on a modestly-sized display. Anyway, happy to be overridden here, but I thought I'd just make that modest suggestion. 😃 |
|
The comments will all be changed to match black's standard, it really is quite comprehensive. If you'd like the line length to be stuck to, then totally up to you! There's a degree of personal preference that doesn't really matter one way or the other. I doubt that black's maintainers did a study or anything to find the sweet spot, and it's not that far off. I'll add a limit for 80 characters. |
|
Sure, though the tool shortcuts I added to the tox configuration format the entire codebase at once. isort and black will have to be called individually to format individual files if you want to do that for a while. If pre-commit is used, then any files subject to the commit will be formatted according to the standard. Since the format configuration is in the A thought about timing for the big commit though: I don't think there ever will be a 'perfect time' for a project like this. In a perfect world, it would be done when there are no PRs pending or any other work being done, but for a public, open-source project, that's only really the case in the beginning, when there's one person working on it. No matter when the commit is made, there's going to be some stepped-on toes and merge conflicts to be resolved, quite a few on my own PRs now. I think that, once you're fine with the tools and have finished experimenting, and are comfortable with it, the best solution is to just take the plunge and make the commit. Unless there's going to be a push to close every outstanding PR, or at least all the major ones, before the formatters are applied, I would cautiously suggest doing it sooner rather than later. If this PR is merged, and people start using the tools, then there will be a lot of commits and PRs with large amounts of formatting changes that only affect a handful of files. That runs counter to the idea of atomic, small commits and will make it much more difficult to review new PRs simply due to the huge diffs that might be generated. It's up to you, but I would suggest that this PR be merged publicly into the master branch close to when you'd like to make the formatting commit, just to reduce these issues as much as possible. Half-formatted code won't make it any easier for the PR reviewers and that is already a bottleneck for the beets project. |
|
These are all really good points. I would love to resolve a couple of the big PRs to avoid some duplicated work—your new typing PRs, for example, are good candidates. Let's make an effort to do this quickly and then merge this configuration (and then do the "big commit"). |
|
One other thing: maybe this PR would be a good place to update any coding guidelines we have in the repository—in |
|
Sure! I'll update the contribution guidelines so that the use of these tools are required. There's also the possibility of adding another check that GitHub runs when PRs are made; this check would enforce the formatting. If the formatting is not according to the standard, the check will fail and stop the PR from being merged, or at least notify the reviewer. |
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Oh overeager bot, this is still relevant but basically involves changing every single line of code at once which won't be fun to deal with for PRs already in progress. |
|
@sampsyo Would you like to pull the trigger on this soon? Not that the UI one has been merged, it might be a good time. |
|
That's a good point. Since we bikeshedded the row-length argument to our satisfaction, and that UI PR is no longer outstanding, maybe it's time! It could be responsible to do a quick audit of open PRs to make sure we're not missing anything obvious we want to do first… but like you say, there will never be a perfectly quiescent moment to do this, so we will eventually need to just take the leap. |
|
Most of the PRs will be (hopefully) pretty easy to merge back in and if not...well like you said, it'll need to happen at some point. Would you like me to add a workflow to this PR? That will make it so that any PR trying to merge with the repository will be required to be fully compliant. It'll mean that once we set it to this, it will (should) never be a problem again, since everyone will have to conform to merge at all. |
pyproject.toml
Outdated
|
|
||
| [tool.isort] | ||
| profile = "black" | ||
| py_version = 36 |
There was a problem hiding this comment.
We should update this to 38 as the last supported version as per Status of Python versions
There was a problem hiding this comment.
That depends on beets. AFAIK beets is still claiming to be backwards compatible to 3.6?
There was a problem hiding this comment.
Alright, I can update the configuration.
Oh yes, that makes sense! Sorry I didn't think of this; it seems really important for keeping this maintainable. I think it would probably be important to replace the style checks from our flake8 checker, lest we get into bad situations where flake8 and Black disagree on style decisions? beets/.github/workflows/ci.yaml Lines 106 to 126 in 7736ae7 Of course, we probably still want the pyflakes (proper linting) component of flake8… |
|
@sampsyo Flake8 and black are definitely compatible. IIRC both of them have options you can set so they work correctly together. I'll double check their docs and make a new workflow and modify the flake workflow if need be. |
Serene-Arc
force-pushed
the
auto_formatters
branch
from
f5dde1f to
8cbe4b4
Compare
|
@sampsyo This adds a workflow that simply runs the tox command/environment 'format_check' which simply checks if there are any changes that either isort or black makes. If there aren't, it passes. Note also that the line length in flake is a limit of 88 characters. This isn't actually the line length that black will try to go to, since black itself is set to 80 characters. However the black docs states that in cases where other rules conflict with the line length, the other rules take precedence e.g. black will make longer lines to preserve readability. You can find that section here. So I've increased the length at which flake errors will occur to account for this but no lines can manually be set to 88 characters. Since black is required to merge, this should result in all lines being as close to 80 characters as possible, and there is no user discretion in that. If you're happy with this, I can run the formatting tools so you can see all the changes before merging. |
|
Sounds good overall! On the topic of flake8 & Black compatibility: it's good to know that they don't actively disagree. But I wonder if we shouldn't remove the flake8 (actually, pycodestyle) checker anyway… that is, we would be saying "the only thing that matters for formatting in the beets project is conforming to what Black would do." So developers could be certain that they only need to run one tool (which you have already helpfully put in a pre-commit hook) and be sure that their style is conformant. Does this make sense? |
|
Yep it does. I can remove the formatting from flake8 if you'd like, or we can phase it out at a later time when we know it won't cause any problems. There should be no change from removing flake8 at any time once black is adopted. If you'd like the continuity of making sure that both are working correctly, it can stay, but if you'd like it removed it probably won't do much differently. I'll have to look at how to remove the formatting checks without changing the linting aspect of the tool. |
|
Sure, sounds right to me! I'm equally happy with turning off pycodestyle in this PR or in a later one—as you say, when we're more confident it won't be disruptive to do so. |
|
Great! Would you like me to make the formatting commit? That's 'the big one'. |
|
Seems like it's about time! The only thing I can imagine doing before we do the "big commit" is giving people some warning. We could tell people via a Discussions post we're going to do it in N days (where N is like 2 or 3) so they have a chance to adapt? (Not sure if the commit comes after this PR merges or before—whatever you see fit!) |
|
I can do it as part of this commit. I'll rebase, which won't have any conflicts, and then do the big format. If you'd like to warn people, it can wait until then! |
|
OK, cool! Sure, why don't we do a short warning period just to minimize disruption and then include the reformat in this PR. Would you mind making the announcement by creating a new Discussions topic and just letting people know what we are planning to do? (I would also be happy to do it, but I think it will make more sense coming from you—and it will let people know that you are in charge. 😃) You can pick any warning duration you see fit. One week from the date of the warning seems perfectly reasonable as a default, for instance. |
|
I'll do that right now :) |
|
Should we do a release before pulling this hammer? |
|
No reason to. This won't impact users at all. Releases won't be affected. |
|
Now that the UI overhaul has been merged, should we consider pulling the trigger on this? |
|
Yes, that sounds good to me! |
|
Alrighty then! Everyone prepare for merge conflicts then |
|
@sampsyo just so it's explicit, do you want me to make the commit and then merge the formatted code to master? |
|
That sounds right to me! Although I admit I don't have strong feelings about the exact process, as long as we end up with "green" CI in the end. 😃 |
|
Then here it gooooooes. |
This is 'the big one', which touches every file so that it all conforms to the given standard.
Serene-Arc
force-pushed
the
auto_formatters
branch
from
3ba89e8 to
a6e5201
Compare
So the last couple issues that have mentioned this have been closed due to staleness so I thought I'd cautiously bring it back up. Now that beets supports Python 3.6+, the auto formatter black and isort can be used on the codebase.
These will standardise all code and hopefully make PRs and merge issues rarer, and improve readability since all formatting will be the same. I have added the tool configurations and a pre-commit file so that that tool can be used as well if desired. These tools make it easy to apply formatting checks as part of the PR workflow and pre-commit makes them run automatically as git is used, so there should be minimal disruption to actual work.
I have not actually reformatted the entire codebase yet. That can be done easily with a tox command but will almost certainly create a large amount of changes through every file, or near enough. If this PR is accepted, that can be done at the end of the process, or when most convenient.
There are some small areas of flexibility in the formatting that may be up for discussion as well. Generally black doesn't offer many choices by design, but the main one is really line length. The war over 80 vs 120 characters can continue here if there's split opinion.