Cleanup/Refactor the code #39
Conversation
Replaced a bunch of single-letter variables with more descriptive ones
…ter per line that string.format
Avoids accidently overriding a python standard function
*Reformatted with autopep8 *renamed input->inp to avoid shadowing input() function
* Formatted with autopep8 * Removed unused import typing.Any * Adding Coding Magic * Renamed argument input->inp to avoid shadowing python builtin * Removed unnecessary dummy variable creation
|
Thanks for your effort. As a very casual Python user, I appreciate the changes to make the code more readable and pythonic, and I'd be happy to merge it as-is. I'm a bit worried this might cause trouble later on for the open pull requests (especially #17) and for anyone who might be working on the feature at the moment... what is your opinion on this? @kvid, @kimmoli and @aakatz3 could maybe chime in with their expertise as well. |
|
I have nothing under work at the moment. Rebasing is ofc needed, but that is what contributors do... Maybe you need to add some guidelines, as the autopep8 is now used. You can run that after all merges, or expect that it is done before pull request. And my nit-pick - this is @formatc1702 to decide, but the commit messages in this PR are kinda mess, and written non imperative. I would squash and rewrite history. See e.g. What is imperative: https://chris.beams.io/posts/git-commit/#imperative rule 5 and why and how to squash: https://medium.com/better-programming/why-and-how-to-squash-git-commits-b508b3b0dba |
|
Squashed, merged, fixed a typo that broke HTML output (stray |
|
I was thinking of updating my PR, but have problems running the latest release after yesterdays refactoring. I have tried creating a new issue about this 4 times now, but they have all failed with some Github server error. |
|
GitHub seems to be having trouble right now... it took me a while to merge this as well. Thanks for your effort, looking forward to your changes. |
* Format all files using autopep8 to add basic PEP8 conformity. * Add Exception types to bare excepts to prevent catching `ctrl+c` * Remove some unnecessary assignment to dummy variables before returning * Add `Optional` to type hints that can be `NoneType` * Change a number of single-letter variables to more descriptive names * Replace string.format() use with Python's f-strings, as they tends to be cleaner, and provide a performance boost. * One multiline string was left as string.format() as I do not believe f-strings support multiline * Some of the string.format() instances had unused/ignored arguments. I left them out of the f-strings, but I marked those cases with a comments that begins `# FIXME:` * Rename variables that were shadowding python standard functions (specifically `format->fmt`, `input->inp`, `type->maintype`) * Some instances of `type` were not changed, as it breaks the yaml parsing. Needs to be looked into. * Move classes in `wireviz.py` to two new files `DataClasses.py` and `Harness.py`. Co-authored-by: Daniel Rojas <github@danielrojas.net>
|
Wow. That was fast. Glad I could be of some help. As my team and I start using this for our project, we will be documenting how we used it, and possibly adding capabilities to it. I hope to be able to push some of our work upstream to you. |
I downloaded this project after hearing about it on Hackaday as I thought that it might solve a problem I have been having on a project. It looks highly certain that it will, but there were a couple of things I was a bit confused about, so I opened up the source code to try to figure things out.
I was having a little trouble understanding some aspects of the code, and my linters were rather unhappy as well, so I set about cleaning it up to conform to PEP8 to help myself understand it.
I thought it might be useful to share the work that I did as a PR.
I have tried my best to split the types of changes up into different commits, so that if certain changes are appreciated, but others are not, I can quickly revert the commit that was that specific change.
The changes:
ctrl+cOptionalto type hints that can beNoneType# FIXME:format->fmt,input->inp,type->maintype)typewere not changed, as it breaks the yaml parsing. Needs to be looked into.wireviz.pyto two new filesDataClasses.pyandHarness.py.I understand that this is kind of a massive amount of changes, some of which are might not be preferred. If some changes are desirable, but others are not, let me know and I can revert the necessary commits to undo the undesirable changes.