pymumble
pymumble copied to clipboard
Cleanup
This PR solves issue #50 – it reformats the code using autoflake
, isort
, and black
(in that order) with all settings default, except:
- isort (to better match black):
--combine-as --trailing-comma --multi-line=3 --line-width 88 --force-grid-wrap 0
- black:
--target-version py37
This further adds type annotations so that mypy
will run using --strict
without errors.
Based on the type annotations some problems were found & fixed.
Lot of changes here. I need to check that, not sure my IDE will like having some import removed, I always find autoflake useless because It remove to much.
Thank for the commit. It's help having the code well formated. I know it's not PEP8, but I don't inderstand why people keep using --line-width 80
today. We don't code on console anymore.
Az
I like good formatting and have submitted PEP8 corrections in the past with manual review, not via tools, and I don't think I agree with all the changes the tools have made. EDIT: Or it seems at least the one I commented on :P I only took a glance at first and it was a glaring one due to the length.
First of all the line length. I feel the same as Azlux and personally use a line length of 120 in most of my projects.
Is the project officially targeting 3.7 or why did you choose that version? If we are instead of going through just the trouble of changing " to ' everything should be changed to f-strings (supported since 3.6) but realistically one might want pymumble to work on 3.4 (Debian 8)
EDIT: FWIW this PR more halves flake8 errors from 514 to 206 and bumps pylint score from -10.14 to 2.17
I don't think I agree with all the changes the tools have made.
sure, me neither, but I think (especially in a shared code situation) the benefits of having a perfect authority (i.e. the configured autoformating tools) completely outweigh personal preferences, which is arguably mostly bikeshedding. I've come to really like having Black in all projects I interact with. You'll never get a "please reformat that line" comment again, or any other kind of argument about formating. It's just done.
Is the project officially targeting 3.7 or why did you choose that version?
Oh, uh, no reason really, I just copied the settings from another project I was working on recently. I didn't actually check what version pymumble is targeting – we should definitely use that version. :)
If we are instead of going through just the trouble of changing " to ' everything should be changed to f-strings (supported since 3.6) but realistically one might want pymumble to work on 3.4 (Debian 8)
The quotation marks are automatically changed by Black though, while f-strings are of course not enforced. Personally I am of the opinion that we should always target a somewhat current version. Old releases don't go away, so if you need to run pymumble on an outdated system, use one of the old releases instead. I think 3.4 should definitely not be supported anymore by new releases of the lib, because python 3.4 itself is not supported anymore and has reached its end-of-life. Current debian comes with python 3.7, while debian9 had py3.5. If we're targeting py>3.5 we should definitely use f-strings, they're great! :D
Lot of changes here.
Yeah, sorry about that. If you go commit by commit it should be easier to check the changes though, I made sure that I only applied the automatic reformating with the first commit, without any code changes at all. All the type annotations are also in a separate commit, although that required very minor code changes already. All the other commits are then fixes based on what mypy reported.
I need to check that, not sure my IDE will like having some import removed, I always find autoflake useless because It remove to much.
I found that it breaks things if you only import a module to expose it as part of your public API without using it yourself, never had any problems with it otherwise and as far as I can tell it also didn't break anything in this case. It's completely optional though, so if you're unsure about it we could just not make it part of the automatic linting/reformating.
Thank for the commit. It's help having the code well formated. I know it's not PEP8, but I don't inderstand why people keep using
--line-width 80
today. We don't code on console anymore.
If it's any help, this is Black's reasoning for their choice of line width (88): https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length