cesbit / aiowmi

Python WMI Queries
GNU General Public License v3.0
27 stars 8 forks source link

First version of wmic_server with Readme #11

Closed majurgens closed 2 years ago

majurgens commented 2 years ago

Description

This is a long-running python server which using aiowmi to make wmi calls on behalf of clients. Clients only need to talk HTTP to this server and hence can be written in many languages. This is in response to discussion I have been having with Rik Lempens. Type of change

[X ] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I have tried many use cases and error cases. Checklist:

[X ] My code follows the style guidelines of this project
[ X] I have performed a self-review of my own code
[ X] I have commented my code, particularly in hard-to-understand areas
majurgens commented 2 years ago

Thank you for the pr!

Please make the code PEP8 compliant. See check result: https://github.com/cesbit/aiowmi/runs/6962415357?check_suite_focus=true

I think it would also be useful to add a contrib/wmic_server/requirements.txt with flask, yaml and aiowmi as requirements.

I will add a requirements.txt file

The formatting requirement for lines less that 79 characters brings me back to the 1980s where I used to use green screen terminals that actually were limited to 80 characters. This formatting requirement seems unrealistic and actually makes the code less readable. I don't know about you but even if I have, what is a small screen today at 1920x1080 I can easily fit 160 characters on a line and still have space to spare. On any monitor today, vertical space is more of a premium than horizontal space. Why force people to scroll up and down to read you code when this is not required? Having to scroll vertically makes code harder to understand and is more of a cognitive load on people, potentially leading to mistakes.

The other requirement for 4 space indentation seems to clash with this max-line-length, requirement, creating longer lines more often.

Then this line, after adding all the other spaces and 4-sapce indentation, pushes the line to 80 characters `config_file = os.getenv('WMIC_SERVER_CONFIG', str(working_dir) + '/wmic_server.yaml')

Even the PEP8 definition calls this line length a suggestion - "PEP 8 suggests lines should be limited to 79 characters.". The max of 79 characters seems a bit archaic.

Here are a couple of examples that actually makes the code (in this case a comment) harder to read and use

The comment is: # PYTHONPATH=/opt/nagios/bin/plugins/aiowmi FLASK_ENV=development WMIC_SERVER_DEBUG=1 python /opt/nagios/bin/plugins/aiowmi/contrib/wmic_server/wmic_server.py which is intended to be a copy and paste, single line command to run the server. Splitting this up would appear to render this useless. In reality this line, when used as a command line will be longer than 79 characters

Another example I just found was this line is 89 characters instead of 79: sys.stdout = io.TextIOWrapper(open(sys.stdout.fileno(), 'wb', 0), write_through=True)

Splitting that line also seems unnecessary.

I'd suggest that the pycodestyle should be run with --max-line-length=200

If they are providing this as a parameter then even they know that it is open to adjustment.

Thoughts?

joente commented 2 years ago

Nope, the code guidelines for this project are PEP8 with the max-line-length included. The mentioned lines are easy to convert to PEP8 style, including the comment. (use \ and place for example each environment variable definition on a new line)

"""
PYTHONPATH=/opt/nagios/bin/plugins/aiowmi \
FLASK_ENV=development \
WMIC_SERVER_DEBUG=1 \
python /opt/nagios/bin/plugins/aiowmi/contrib/wmic_server/wmic_server.py
"""

For the other example a similar solution is possible:

sys.stdout = \
    io.TextIOWrapper(open(sys.stdout.fileno(), 'wb', 0), write_through=True)
majurgens commented 2 years ago
sys.stdout = \
    io.TextIOWrapper(open(sys.stdout.fileno(), 'wb', 0), write_through=True)

Sorry but I am really struggling with this one and need some more help. Your solution is still one character too long.

So I tried

sys.stdout = io.TextIOWrapper\
   (open(sys.stdout.fileno(), 'wb', 0), write_through=True
```)

But that complains there is whitespace before the "(" and of course you need the whitespace otherwise it complains about indentation.

So then I tried

sys.stdout = io.TextIOWrapper(open(sys.stdout.fileno(), 'wb', 0), \ write_through=True



But that complains that the backslash is redundant between brackets

The solution appears to be to not use the continuation operator "\" and write
    sys.stdout = io.TextIOWrapper(open(sys.stdout.fileno(), 'wb', 0),
                                  write_through=True)

but you have to make sure that there is exactly 34 spaces (not a multiple of 4) at the start of the 2nd line. One more space and it becomes "over-indented" and one less space and it becomes "under-indented".

There are 2 other lines I have in the same condition but these ones require exactly 31 and 33 spaces. I find this a little unusual. I am not sure what is going on.

Is there a better solution since it seems like having to put an exact number of spaces, that varies per instance is a really weird requirement?