Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to using an object for the arguments and accept a new arpPath argument #59

Merged
merged 6 commits into from Aug 15, 2022

Conversation

timrogers
Copy link
Collaborator

@timrogers timrogers commented May 17, 2022

This updates the API of the findLocalDevices function to allow you to optionally specify the path of the arp executable using the arpPath option, rather than assuming that it is available in the PATH.

As part of this change, I make a breaking change to the API of the function, switching from positional arguments to a keyed object. This makes the API more friendly and discoverable as the number of options grows, and means you don't have to use nulls to say "I don't want to set this option".

It also corrects the findLocalDevices function type to reflect the skipNameResolution argument which is already accepted.

@DylanPiercey
Copy link
Owner

@timrogers i think this looks good. Could you update the docs?

src/index.d.ts Outdated Show resolved Hide resolved
@timrogers timrogers changed the title Allow specifying the path of the arp binary Switch to using an object for the arguments and accept a new arpPath argument May 18, 2022
@timrogers
Copy link
Collaborator Author

@DylanPiercey 👋🏻 Hope you're having a good weekend! Any chance we can get this merged?

@timrogers
Copy link
Collaborator Author

@DylanPiercey 👋🏻 Just a reminder about this PR when you have a second!

@DylanPiercey
Copy link
Owner

Hey @timrogers @natterstefan! I'm having a hard time maintaining this project since I haven't actively used it in years and I don't have a lot of confidence in its test suite (eg we're not testing against windows).

Wondering if either of you would like to step in as a maintainer on GitHub (I know @natterstefan already is) with npm publish access also?

@timrogers
Copy link
Collaborator Author

I would be happy to take it on. I don't realistically have huge amounts of time to drive the project forward, but I can at least steward it and review things.

src/index.d.ts Outdated Show resolved Hide resolved
@natterstefan
Copy link
Collaborator

Hey @DylanPiercey,
Hey @timrogers,

Wondering if either of you would like to step in as a maintainer on GitHub (I know @natterstefan already is) with npm publish access also?

I'm sorry I missed lots of PRs too. Additionally, I was not exactly sure how we proceed with PRs (do we both need to accept it or just one of us) and how we proceed with releases after merges. I am happy to step in as a more active maintainer (together with @timrogers if he wants) proceeding more actively PRs etc. up to a release on npm.

I don't have a lot of confidence in its test suite (eg we're not testing against windows).

Me too with the windows part, but I could set up at least GitHub actions for Linux, macOS, and Windows and execute the command in a native environment.

@timrogers
Copy link
Collaborator Author

@DylanPiercey Ready for re-review!

@DylanPiercey
Copy link
Owner

Looks good, thanks!

@DylanPiercey DylanPiercey merged commit 73a2145 into DylanPiercey:master Aug 15, 2022
@DylanPiercey
Copy link
Owner

Published as 4.0.0

@natterstefan
Copy link
Collaborator

Thank you, @DylanPiercey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants