fix: port from identity file #36
Conversation
WalkthroughThe changes in the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
hivemind_bus_client/scripts.py (1)
Line range hint
104-106: Add missing port fallback logic in send_mycroft functionOther functions use a consistent port fallback pattern, but
send_mycroftis missing this logic. This could lead to inconsistent behavior.Add the port fallback logic before creating the client:
def send_mycroft(key: str, password: str, host: str, port: int, siteid: str, msg: str, payload: str): + identity = NodeIdentity() + port = port or identity.default_port or 5678 node = HiveMessageBusClient(key, host=host, port=port, password=password)
🧹 Nitpick comments (2)
hivemind_bus_client/scripts.py (2)
23-23: Update help text to match the new port parameter behaviorThe help text for the port option still mentions "default: 5678" but the Click option is now optional without a default value. The default is actually handled in the function implementation. Consider updating the help text to be more accurate:
-@click.option("--port", help="HiveMind port number (default: 5678)", type=int, required=False) +@click.option("--port", help="HiveMind port number (defaults to value from identity file or 5678)", type=int, required=False)Also applies to: 48-48, 99-99, 120-120, 157-157
Line range hint
51-67: Consider extracting common initialization logicThere's significant code duplication in the identity initialization, validation, and connection setup across functions. Consider extracting this into a helper function.
def initialize_hivemind_client(key: str = "", password: str = "", host: str = "", port: Optional[int] = None, siteid: str = "") -> HiveMessageBusClient: """Initialize HiveMind client with identity fallbacks and validation.""" identity = NodeIdentity() password = password or identity.password key = key or identity.access_key host = host or identity.default_master siteid = siteid or identity.site_id or "unknown" port = port or identity.default_port or 5678 if not host.startswith("ws://") and not host.startswith("wss://"): host = "ws://" + host if not key or not password or not host: raise RuntimeError("NodeIdentity not set, please pass key/password/host or " "call 'hivemind-client set-identity'") node = HiveMessageBusClient(key, host=host, port=port, password=password) node.connect(FakeBus(), site_id=siteid) return nodeThis would simplify the command functions to:
def terminal(key: str, password: str, host: str, port: Optional[int], siteid: str): node = initialize_hivemind_client(key, password, host, port, siteid) node.connected_event.wait() print("== connected to HiveMind") # ... rest of the functionAlso applies to: 124-139, 161-176
| @click.option("--port", help="default port for hivemind-core", type=int, required=False) | ||
| @click.option("--siteid", help="location identifier for message.context", type=str, default="") | ||
| def identity_set(key: str, password: str, host: str, port: int, siteid: str): | ||
| if not key and not password and not siteid: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update type hints for optional port parameter
The port parameter is now optional, but the type hints still show it as required int. Update the type hints to reflect this change.
-def identity_set(key: str, password: str, host: str, port: int, siteid: str):
+def identity_set(key: str, password: str, host: str, port: Optional[int], siteid: str):Don't forget to add the import:
+from typing import OptionalAlso applies to: 51-51, 104-104, 124-124, 161-161
Summary by CodeRabbit
New Features
portparameter non-required for several commands, enhancing flexibility in usage.5678when no port is provided.Bug Fixes