feat: integrate findlinx with the find command #61
Conversation
Reviewer's Guide by SourceryThis pull request integrates the 'find' command into the CLI tool, allowing users to discover NetLinx devices on a local broadcast subnet. The implementation includes new classes and types to support this functionality, as well as updates to the documentation and dependencies. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @damienbutt - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| }); | ||
|
|
||
| return new Promise((resolve) => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
suggestion: Consider making the discovery timeout configurable
The current 6-second timeout might not be sufficient in all network conditions. Consider making this a configurable option, perhaps with a default value of 6 seconds.
setTimeout(() => {
socket.close();
}, this.config.discoveryTimeout || 6000);
| export async function discover(): Promise<Array<Device>> { | ||
| const devices: Array<Device> = []; | ||
|
|
||
| const socket = dgram.createSocket("udp4"); |
There was a problem hiding this comment.
suggestion: Add error handling for socket operations
Consider adding error handling for socket operations. For example, you could add event listeners for 'error' and 'close' events to handle potential issues gracefully.
| const socket = dgram.createSocket("udp4"); | |
| const socket = dgram.createSocket("udp4"); | |
| socket.on('error', (err) => { | |
| console.error('Socket error:', err); | |
| socket.close(); | |
| }); | |
| socket.on('close', () => { | |
| console.log('Socket closed'); | |
| }); |
|
|
||
| const ICSP_PORT = 1319; | ||
|
|
||
| export async function discover(): Promise<Array<Device>> { |
There was a problem hiding this comment.
suggestion: Consider adding a fallback mechanism for device discovery
The current implementation uses UDP broadcast, which might not work in all network configurations. Consider adding a fallback mechanism or a way to specify IP ranges for more flexible device discovery.
export async function discover(options?: DiscoveryOptions): Promise<Array<Device>> {
const devices: Array<Device> = [];
const { useUdp = true, ipRanges = [] } = options || {};
if (useUdp) {
devices.push(...await discoverViaUdp());
}
for (const range of ipRanges) {
devices.push(...await discoverViaIpRange(range));
}
|
🎉 This PR is included in version 2.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by Sourcery
Integrate a new 'find' command to discover NetLinx devices on a local broadcast subnet, update dependencies, and enhance documentation.
New Features:
Enhancements:
Documentation: