search

DylanPiercey / local-devices

🔮 Find devices connected to the current local network.
MIT License
139 stars 27 forks source link

fix: add timeout options when exec arp #13

Closed howel52 closed 5 years ago

howel52 commented 5 years ago

https://github.com/DylanPiercey/local-devices/issues/12

howel52 commented 5 years ago

Thank you for your PR and help @howel52. I have just one thing I'd suggest to change.

@DylanPiercey Do you agree with the value for the timeout? Edit: and we might need to think about a way developers can define the timeout and maxBuffer themselves. But that's most likely out of scope of this PR.

yeah, I think developers define the options by themselves is right.

And now, In this PR, I have modified the code for suggestions on code review. @natterstefan

DylanPiercey commented 5 years ago

@howel52 thanks for this PR, it looks great 🙂.

I agree with @natterstefan that this should be configurable, but it isn’t absolutely required that we do that in this PR.

This change will of course require a major release since the current default timeout is effectively Infinity. We can then do a minor release to expose both the max buffer and timeout.

howel52 commented 5 years ago

I want to know when this PR can be merged. And when will the new npm package be released ?

natterstefan commented 5 years ago

Hi @howel52.

I have just merged it, but it's up to @DylanPiercey to release a new version of the package.

@DylanPiercey we could then probably also include (merge+release) this PR: https://github.com/DylanPiercey/local-devices/pull/9.

howel52 commented 5 years ago

Hi @howel52.

I have just merged it, but it's up to @DylanPiercey to release a new version of the package.

@DylanPiercey we could then probably also include (merge+release) this PR: #9.

thx :)