Show favorite nodes in --nodes #888
Conversation
|
Hm. I've been reluctant to add columns to this display, since of course you can just use |
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.
| 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 == "" |
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.
| @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, | ||
| }, | ||
| } |
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.
| iface = MeshInterface(noProto=True) | ||
| iface.nodes = nodesById | ||
| iface.nodesByNum = nodesByNum | ||
| from unittest.mock import MagicMock |
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.
|
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. |
|
I see the pylint failures, will review. |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@ianmcorvidae Addressed pylint and the unittest suggestions from copilot. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
2 participants
With CLIENT_BASE, favorites become more important, so I added a column to mark favorited nodes in the
--nodestable output. I realize it's getting a bit long, so I abbreviated it to 'Fav'.