Show favorite nodes in --nodes - #888
Conversation
Sorry, something went wrong.
|
Hm. I've been reluctant to add columns to this display, since of course you can just use --show-fields to show whatever. But I do think that favorites have become important enough to warrant this. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR adds a "Fav" (favorite) column to the --nodes table output to make it easier to identify favorited nodes, which becomes more important with CLIENT_BASE. The implementation adds display logic for the new isFavorite field that shows an asterisk (*) for favorite nodes and an empty string for non-favorite nodes.
Key changes:
- Added "isFavorite" field to default showFields list in showNodes method
- Added "Fav" column header mapping in get_human_readable function
- Added formatting logic to display "*" for favorite nodes and "" for non-favorites
- Added comprehensive test suite with 5 test cases covering various scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| meshtastic/tests/test_showNodes_favorite.py | New test file with comprehensive test coverage for the favorite column feature including header display, asterisk formatting, custom fields, and default fields |
| meshtastic/mesh_interface.py | Updated showNodes method to include isFavorite in default fields, added "Fav" column header mapping, and added formatting logic to display favorites with an asterisk |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| def test_showNodes_favorite_asterisk_display(capsys, iface_with_favorite_nodes): | ||
| """Test that favorite nodes show asterisk and non-favorites show empty""" | ||
| iface = iface_with_favorite_nodes | ||
| iface.showNodes() | ||
| out, err = capsys.readouterr() | ||
|
|
||
| # Check that the output contains the "Fav" column | ||
| assert "Fav" in out | ||
|
|
||
| # The favorite node should have an asterisk in the output | ||
| # We can't easily check the exact table cell, but we can verify | ||
| # the asterisk appears somewhere in the output | ||
| lines = out.split('\n') | ||
|
|
||
| # Find lines containing our nodes | ||
| favorite_line = None | ||
| regular_line = None | ||
| for line in lines: | ||
| if "Favorite Node" in line or "FAV1" in line: | ||
| favorite_line = line | ||
| if "Regular Node" in line or "REG1" in line: | ||
| regular_line = line | ||
|
|
||
| # Basic sanity check - if we found the lines, they should be present | ||
| assert favorite_line is not None or regular_line is not None | ||
| assert err == "" |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
This test claims to verify that favorite nodes show asterisk and non-favorites show empty, but the actual assertions at the end only check that favorite_line or regular_line is not None, without verifying the presence or absence of asterisks. The test should actually verify that an asterisk appears in favorite_line when it's found, or should be more explicit about what it's testing. Consider adding assertions that check for asterisk presence in favorite_line and absence in regular_line.
Sorry, something went wrong.
| @pytest.fixture | ||
| def iface_with_favorite_nodes(): | ||
| """Fixture to setup nodes with favorite flags.""" | ||
| nodesById = { | ||
| "!9388f81c": { | ||
| "num": 2475227164, | ||
| "user": { | ||
| "id": "!9388f81c", | ||
| "longName": "Favorite Node", | ||
| "shortName": "FAV1", | ||
| "macaddr": "RBeTiPgc", | ||
| "hwModel": "TBEAM", | ||
| }, | ||
| "position": {}, | ||
| "lastHeard": 1640204888, | ||
| "isFavorite": True, | ||
| }, | ||
| "!12345678": { | ||
| "num": 305419896, | ||
| "user": { | ||
| "id": "!12345678", | ||
| "longName": "Regular Node", | ||
| "shortName": "REG1", | ||
| "macaddr": "ABCDEFGH", | ||
| "hwModel": "TLORA_V2", | ||
| }, | ||
| "position": {}, | ||
| "lastHeard": 1640204999, | ||
| "isFavorite": False, | ||
| }, | ||
| } | ||
|
|
||
| nodesByNum = { | ||
| 2475227164: { | ||
| "num": 2475227164, | ||
| "user": { | ||
| "id": "!9388f81c", | ||
| "longName": "Favorite Node", | ||
| "shortName": "FAV1", | ||
| "macaddr": "RBeTiPgc", | ||
| "hwModel": "TBEAM", | ||
| }, | ||
| "position": {"time": 1640206266}, | ||
| "lastHeard": 1640206266, | ||
| "isFavorite": True, | ||
| }, | ||
| 305419896: { | ||
| "num": 305419896, | ||
| "user": { | ||
| "id": "!12345678", | ||
| "longName": "Regular Node", | ||
| "shortName": "REG1", | ||
| "macaddr": "ABCDEFGH", | ||
| "hwModel": "TLORA_V2", | ||
| }, | ||
| "position": {"time": 1640206200}, | ||
| "lastHeard": 1640206200, | ||
| "isFavorite": False, | ||
| }, | ||
| } |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
The test fixture should include a node that doesn't have the isFavorite field at all to test backward compatibility with existing nodes. Currently, all test nodes explicitly have isFavorite set to either True or False. Adding a third node without the isFavorite field would ensure the implementation handles legacy nodes gracefully.
Sorry, something went wrong.
| iface = MeshInterface(noProto=True) | ||
| iface.nodes = nodesById | ||
| iface.nodesByNum = nodesByNum | ||
| from unittest.mock import MagicMock |
Copilot
AI
Jan 8, 2026
There was a problem hiding this comment.
The import statement should be moved to the top of the file with the other imports for consistency with the project's style. Other test files in this codebase import MagicMock at the module level rather than within functions.
Sorry, something went wrong.
|
Copilot's review looks to be stuff worth changing, plus you'll need to ensure pylint/mypy are happy as well (that being the CI build failure). But I think it's worth the change, anyway. |
Sorry, something went wrong.
|
I see the pylint failures, will review. |
Sorry, something went wrong.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Sorry, something went wrong.
|
@ianmcorvidae Addressed pylint and the unittest suggestions from copilot. |
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #888 +/- ##
==========================================
+ Coverage 59.88% 59.93% +0.04%
==========================================
Files 24 24
Lines 4318 4320 +2
==========================================
+ Hits 2586 2589 +3
+ Misses 1732 1731 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
With CLIENT_BASE, favorites become more important, so I added a column to mark favorited nodes in the --nodes table output. I realize it's getting a bit long, so I abbreviated it to 'Fav'.