-
Notifications
You must be signed in to change notification settings - Fork 268
Add optional CLI parameter to specify node info fields to show with --nodes parameter. #736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I've given this PR a test drive and functionally it works well. I'll let a dev comment on the code and structure. Nice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caught one small thing, and also set the CI to run. We'll see if that turns anything up, but otherwise this looks good to me! I'm sure there's some stuff we could do to make it more concise or smart, but I think this way is good for now.
meshtastic/mesh_interface.py
Outdated
return None | ||
return value | ||
|
||
if showFields is None or showFields.count == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure that count
is a method on lists, not a property, so this wouldn't ever be true. I'm guessing you mean len(showFields) == 0
here.
Ah, yeah, looks like pylint is complaining a bit. If you want to check the CI stuff locally, you can run:
and and It looks like I may have messed something up with mypy, though, just pushed a fix for that to master. So if it complains about |
Want to run something by you. Running pylint, I see:
But I'm specifically taking advantage of Python's dynamic typing here to save a line or two. I "fixed" it by indicating that it could be a str, dict or even None: value: Optional[Union[str, dict]] = node_dict |
I think that's fine, particularly given the return value for the function in general is |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #736 +/- ##
==========================================
+ Coverage 60.13% 60.60% +0.46%
==========================================
Files 24 24
Lines 3986 4031 +45
==========================================
+ Hits 2397 2443 +46
+ Misses 1589 1588 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Fixed up a couple small things that pylint/pytest were complaining about since I know you were having some install issues from discord. I think this looks good to me, thanks for the contribution! |
This PR adds the ability to specify which node information is shown in the table produced by the
--nodes
parameter. It is optional, and specifying the--nodes
parameter without the new param outputs the same table it does today.The new CLI param is
--show-fields
and it requires a comma-separated list of field names. It can only be specified alongside--nodes
.For example,
--nodes --show-fields user.id,user.hwModel,position.altitude
will output the following table (the "N" column always shows):I also did a bit of refactoring in the
showNodes()
function inmesh_interface.py
related to this. Rather than simply going through and retrieving individual bits of information out of the data structures, there's now a default list of fields to show and that list contains the fields that are shown today.There's now a mapping of field names to human-readable names. Fields that are not in the mapping will be shown with raw field names and values, for example:
--nodes --show-fields user.id,user.hwModel,position.altitude,deviceMetrics.voltage
will show the following until someone goes in and adds the name mapping and value formatting. Note the column header name is not human-readable, the actual values are not formatted to a particular precision, and there's no unit of measure shown.