-
Notifications
You must be signed in to change notification settings - Fork 81
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
Minimum supported Python version & formatting #624
Comments
I'm not a fan of 80 char line requirements but they're mostly specified by PEP so it is what it is, I guess. But I don't think we should adhere to it so adamantly.
I thought of doing this long ago and I fully agree. The current messenger system is just a leftover because this program was originally written on Perl then ported over. Python's logging module is much cleaner and will allow for the current functionality and more (logging to file, logging to a window, etc.)
I'm also a fan of the newer f-string style (I wasn't a fan of .format which is why I left a mess in there) so I'd be committed to switching to those fully. Type hints also look very clean to me and being a fan of typed languages in the first place I'd love having it implemented in the codebase.
I agree. 100 is fine in my book.
Let's do that first. I was gone for a good while because I was forced to use Windows for work but now that I'm back on Linux (and also watching anime again) I have an environment and motivation to go forward. It'd be good to clean everything up before fixing or adding more features, and by cleaning up I don't mean only style issues but also some really weird or hard to understand code, particularly in the media player spawning code, tracker code, etc. |
So, black still has some issues or follows some code style that I personally don't agree with (e.g. psf/black#2156), but I don't really have strong feelings against them. For my personal projects, I'd still use my own code style, but for something I collaborate with others on, having a tool-enforced standard just makes everyone's lifes so much easier. Black can also be combined with μsort (using μfmt, for example). Another alternative to black is yapf. I'd probably run both on the code base, compare their results and then go with the one we like best. As for other transformations that we should certainly do, pyupgrade is an easy choice to remove old pythonism and replace them with newer ones. I'd also like to add type hints for mypy, but that doesn't have to be a full update and can instead also be done step by step. In general, I'd like to merge as many open pull requests as possible before making such a huge change because of the inevitable merge conflicts that it will create, but for smaller PRs it's not hard to rebase/merge them. |
We should also stop relying on |
Related: #609 |
Haven't made any progress on this part, but I wanted to mention that since the initial posting of this issue, ruff has also established itself as a very performant alternative to black with a bit more configuration, so I would favor that at the moment. It does not sort imports (yet), but neither does black as far as I'm aware and the linter actually can auto-fix them. |
The current code base is in a bit of a mess with its coding style of seemingly random line breaks to adhere to imho ridiculous line length requirements. Additionally, string formatting switches between % and str.format all the time and we might consider using the logging module instead of the custom messaging system as well.
As of today, the oldest Python version that has not reached end of life is 3.7, which I strongly suggest to adopt as our minimum supported version, too. This will mean that we get access to some additions to the standard library as well as f-strings and type hints. There are tools to help with the migration to newer versions that I'd like to test & apply to the entire code base.
Regarding formatting, I have a personal preference on a couple of things but generally like to follow PEP8 with a focus on keeping line breaks clean from git diff collisions, i.e. always add trailing commas/punctuation where possible and binary operators after the line break. As for line length, 80 is ridiculous in my point of view. 100 is quite workable and 120 would be the maximum I'd allow for.
Using an auto-formatter like
black
and enforcing it in some fashion (e.g. through a pre-commit hook) will probably go a long way here. Ideally after most open pull requests have either been merged or rejected because it will cause conflicts with a lot of them.The text was updated successfully, but these errors were encountered: