Cleanup #51
Cleanup #51
Conversation
includes some minor code changes to help mypy understand the code, but does *not* yet fix any problems/broken code!
get_childs returns a list, not a dict
|
Lot of changes here. 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 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 |
| } | ||
| ) | ||
|
|
||
| def set_callback(self, callback: str, dest: _Callback) -> None: |
There was a problem hiding this comment.
I feel like this change is a rather subjective one. It just makes the lines longer because the tool decided you need to have this hierarchy.
There was a problem hiding this comment.
yup, I completely agree with you. I would never format it like this manually.
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.
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. :)
The quotation marks are automatically changed by Black though, while f-strings are of course not enforced. |
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 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.
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 |
This fixes problems with mypy & possibly other tools that ignore incidentally exported names.
Generated files shouldn't be changed, not even automatically.
While the Protobuf does need nested objects, the Cmd is supposed to store only IDs. The code that creates the Protobuf from the Cmd is aware of this and properly creates the nested objects accordingly.
This PR solves issue #50 – it reformats the code using
autoflake,isort, andblack(in that order) with all settings default, except:--combine-as --trailing-comma --multi-line=3 --line-width 88 --force-grid-wrap 0--target-version py37This further adds type annotations so that
mypywill run using--strictwithout errors.Based on the type annotations some problems were found & fixed.