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

Add actions for getting 1 or 2 sockets #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chris-martin
Copy link

Requiring exactly one or two sockets seems like a really common use case, so I thought we could support it explicitly, with good error messages for when an unexpected number of sockets is present.

I also added a re-export of the Socket type here for convenience.

@ddfisher
Copy link
Owner

Thanks for the PR! I'll try to find some time this week to take a more detailed look at it, but here are some initial thoughts:

  • Overall, convenience methods for getting one or two activated sockets seem like a nice thing to have, so I'd be glad to merge some version of this PR.
  • From a quick look, the implementation looks good.
  • I'm not in love with the names for the new methods. I like that they're quite specific, but they also feel a bit clunky/verbose. I don't have any concrete alternatives, though.
  • I'm not sure I like re-exporting Socket -- it doesn't feel idiomatic to me. Are there other libraries that do similar things?

@chris-martin
Copy link
Author

I considered getActivatedSockets1 and getActivatedSockets2, would that sound better?

Some context for why I wanted the Socket re-export: I was just passing the Socket straight into a function from another library, I ended up having to add a network dependency to my project just for the sole purpose of importing Socket so I could write an expression type-signature, which seemed sort of ridiculous. I don't really have a strong opinion, and there doesn't seem to be any popular consensus on the subject, so I'm happy to take it out if you don't want it.

@chris-martin
Copy link
Author

chris-martin commented Jun 7, 2018

Updated:

  • Removed the Socket re-export
  • Renamed the functions to getActivatedSockets1 and getActivatedSockets2
  • Added example usage docs for all three functions

@chris-martin chris-martin force-pushed the pr/fixed branch 3 times, most recently from b904833 to 5c10fd2 Compare June 7, 2018 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants