-
Notifications
You must be signed in to change notification settings - Fork 52
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
Enable TCP connections and export createClient method #23
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,7 +32,9 @@ function createStream(opts) { | |
try { | ||
switch (family.toLowerCase()) { | ||
case 'tcp': | ||
throw new Error('tcp dbus connections are not supported'); | ||
host = params.host || 'localhost'; | ||
port = params.port; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I like the idea of being able to override address string properties with option parameters because then we have to do it for every parameter. Just specify it in the address like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, that's actually what this is doing, my mistake. I mistook params for opts. 👍 |
||
return net.createConnection(port, host); | ||
case 'unix': | ||
if (params.socket) { | ||
return net.createConnection(params.socket); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to expose this. Just use
sessionBus()
. It's the same thinghttps://github.com/acrisci/node-dbus-next/blob/a241269c6e917e22ae6e9f1015713751192469ff/index.js#L40-L42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but that's a bit of a strange way to access the system bus...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can allow to override the address for
systemBus()
if you want. Or propose to have only one function to create a bus.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this actually what I'm doing here, other than leaving two 'convenience' functions for
systemBus
andsessionBus
? Removing those would be quite a major breaking change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I'm still probably about 2 months from a stable api and have some other major breaking changes planned that are worse than this. It's something I want to get right because I'm copying this pattern in the python version of this library.
I think one or two functions would be a good idea, but three is too many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way this looks actually. Open a new issue for it though because there are some subtle points.