Skip to content
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

Open
FichteFoll opened this issue Jul 3, 2022 · 4 comments
Open

Minimum supported Python version & formatting #624

FichteFoll opened this issue Jul 3, 2022 · 4 comments
Labels

Comments

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jul 3, 2022

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.

@z411
Copy link
Owner

z411 commented Jul 31, 2022

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.

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.

we might consider using the logging module instead of the custom messaging system as well.

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.)

his will mean that we get access to some additions to the standard library as well as f-strings and type hints.

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.

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.

I agree. 100 is fine in my book.

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.

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.

@FichteFoll
Copy link
Collaborator Author

FichteFoll commented Aug 5, 2022

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.

@txtsd
Copy link
Contributor

txtsd commented Oct 25, 2022

We should also stop relying on setup.py, and use requirements.txt, orpipenv, or poetry to create a local dev environment.

@FichteFoll
Copy link
Collaborator Author

Related: #609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants