Skip to content

Create SysCmds common class, and refactor Python codebase#697

Merged
rdmark merged 23 commits intodevelopfrom
rdmark-sys-cmds-class
Feb 27, 2022
Merged

Create SysCmds common class, and refactor Python codebase#697
rdmark merged 23 commits intodevelopfrom
rdmark-sys-cmds-class

Conversation

@rdmark
Copy link
Copy Markdown
Member

@rdmark rdmark commented Feb 22, 2022 •
edited
Loading

  • Move most pi_cmds methods into a new SysCmds class in the common package
  • Renamed web/src/device_tools to web_tools; move auth_active method from pi_cmds to web_utils
  • Break out some more functionality out of web.py into utility modules
  • Resolve or suppress most pylint warnings
  • Moved inline library imports up to the top level in each module. This increases the memory usage of the python3 process when the Web Interface is processing commands by 1.8k (29.54k -> 31.39K)
  • Reenable the import-outside-toplevel pylint rule
  • Tweak the Web UI "RaSCSI" banner heading. Requested by @akuker
  • Display the hostname in the page title and top banner. Requested by @bzeiss

rdmark added 13 commits February 22, 2022 09:01
@rdmark
Move the oled script's PiCmds module to common, and rename it SysCmds.
407d00b
@rdmark
Use sys_cmds.get_ip_and_host() in web UI code.
d9ad99c
@rdmark
Move the auth_active() method to device_utils
6b907bb
@rdmark
Rename device_utils to web_utils. Make auth_active() method take the …
03ddd4a 
…group as argument.
@rdmark
Migrate all pi_cmds methods to the SysCmds common class.
7f58fa3
@rdmark
Display hostname and ip in Web UI.
bf21937
@rdmark
Resolve or suppress pylint warnings.
fd7a86d
@rdmark
Resolve a pylint warning.
3d674c0
@rdmark
Resolve or suppress pylint warnings.
b1b1130
@rdmark
Import libraries at the top level for readability. In my testing on a…
c1ef8fa 
… Pi3B+, this leads to ~1.5k more memory being used by the python3 process.
@rdmark
Change page title as requested by akuker.
9694156
@rdmark
Reenable the import-outside-toplevel pylint rule.
6f55d2a
@rdmark
Resolve pylint warnings.
ea0c97d
@rdmark rdmark changed the base branch from master to develop February 22, 2022 20:58
@rdmark rdmark requested a review from bzeiss February 22, 2022 20:58
@bzeiss
Copy link
Copy Markdown
Collaborator

bzeiss commented Feb 22, 2022

@rdmark These look like nice changes wrt. overall quality improvements. I have nothing specific to add to the code comments specifically. It's probably worth to give this branch a practical test run once you consider it complete.

@rdmark rdmark marked this pull request as ready for review February 23, 2022 02:21
@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Feb 23, 2022

@bzeiss Thanks for having a look! Right now only the get_ip_and_host() class method is shared by two clients (web and oled) but hopefully other SysCmds methods can be shared and expanded upon, perhaps as your control board interface matures.

I wanted to refactor logging in line with #530 but couldn't get it to work as I liked after an hour of trying, so shelving it for another day and another PR.

This is now ready for formal review/sign-off. :)

@rdmark rdmark marked this pull request as draft February 23, 2022 07:46
@rdmark rdmark marked this pull request as ready for review February 23, 2022 07:54
@bzeiss
Copy link
Copy Markdown
Collaborator

bzeiss commented Feb 26, 2022

@rdmark Finally had a chance to give this a test run. This is what I got which I find a little odd. I had to delete the venv in web in order to make it work. I'm starting to suspect that it really is a good idea to clean up the venv's after a new easyinstall.sh call for all python projects. Other than that, I didn't see anything broke. Thanks again for the hostname in the banner :-)

Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]: Traceback (most recent call last):
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:   File "/home/pi/RASCSI/python/web/venv/bin/pybabel", line 6, in <module>
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:     from babel.messages.frontend import main
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:   File "/home/pi/RASCSI/python/web/venv/lib/python3.7/site-packages/babel/messages/frontend.py", line 31, in <module>
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:     from babel.messages.pofile import read_po, write_po
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:   File "/home/pi/RASCSI/python/web/venv/lib/python3.7/site-packages/babel/messages/pofile.py", line 1
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:     //---------------------------------------------------------------------------
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]:      ^
Feb 26 10:11:59 raspberrypi RASCSIWEB[7477]: SyntaxError: invalid syntax

@rdmark
Copy link
Copy Markdown
Member Author

rdmark commented Feb 27, 2022

@bzeiss Thanks for testing! Yes, venv is very fragile. Doing some cleanup in easyinstall isn't a bad idea. I wonder where the //-------------------- came from though...?

@rdmark rdmark merged commit e8f392c into develop Feb 27, 2022
@rdmark rdmark deleted the rdmark-sys-cmds-class branch February 27, 2022 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bzeiss bzeiss bzeiss approved these changes

@akuker akuker Awaiting requested review from akuker akuker is a code owner

@erichelgeson erichelgeson Awaiting requested review from erichelgeson

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rdmark @bzeiss