Closed liaden closed 5 years ago
Hi @liaden, currently this is mostly a lib, the main program is really just a test one, it just blindly dump the output of several pynetgear function calls.
I am fine if you want to improve it so it can be used standalone, but then you need to add a bit more than that, at least a switch to specify what you want to get (list of devices, traffic meter ?). It makes no sense to unstructurally dump the output of random functions like my current main() for a user perspective.
But if you have the will to do it I'll gladly merge that.
Also little tip for the formatting errors, you can easily check that locally by doing the same as in .travis.yml:
pip install flake8
flake8 pynetgear
@MatMaul I was definitely was thinking that exposing the other methods would be nice.
Also, thanks for the heads up about flake8
. I've been away from python for several years now.
@MatMaul Ignoring the line too long at the moment, I wanted to make sure this is a good direction from your perspective.
Below is what the help command looks like:
python3 -m pynetgear -h
usage: pynetgear [-h] [--format {json,py}] [--ssl] [--host HOST] [--user USER]
[--port PORT] [--url URL]
{block_device,allow_device,login,attached_devices,traffic_meter}
...
optional arguments:
-h, --help show this help message and exit
--format {json,py}
router connection config:
--ssl Connect with https
--host HOST Hostname for the router
--user USER Account for login
--port PORT Port exposed on the router
--url URL ....
subcommands:
Runs subcommand against the specified router
{block_device,allow_device,login,attached_devices,traffic_meter}
block_device Blocks a device from connecting by mac address
allow_device Allows a device with the mac address to connect
login Attempts to login to router.
attached_devices Outputs all attached devices
traffic_meter Output router's traffic meter data
@liaden after a quick look looks good :) I'll have a closer look later. Disregards lines too long, it's false warnings... The limit is 120 (cf setup.cfg), I don't know why Hound says otherwise...
@MatMaul I looked back over it this morning and spotted a few more things (like missing password for cli). I also was thinking about defaulting to using ssl and making the flag --no-ssl
to disable it.
Added password and url that was missing from the CLI and a little bit of cleanup. I can squash and cleanup the commit message prior to merging if needed.
@liaden globally it looks quite good ! Adding some small comments.
Actually the typo is my only comment :+1: :+1: If it's tested it is good for inclusion, I'll do some quick tests later today, if you can fix the typo and squash everything I'll merge it afterwards.
Squashed, and typo fixed.
@MatMaul anything else outstanding on this that you want me to address?
@liaden awesome thanks. Merging this, and pushing some small fixes and pretty json print on top of it :)
I am wanting to run this to generate some json that I can then plug into my json attributes for chef runs when configuring my home lab. It feels a bit nicer to have the pynetgear output json directly instead of using another tool to convert it.