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

Reduce API variability due to system configuration #53

Closed
wants to merge 4 commits into from
Closed

Reduce API variability due to system configuration #53

wants to merge 4 commits into from

Conversation

bmillwood
Copy link
Contributor

Now all constructors of SocketOption, Family, and SocketType will always
be declared, but using ones not supported on your system will lead to
runtime errors. To guard against these, new functions isSupportedFamily
etc. are introduced.

As a side effect, DummySocketOption__ goes away, so this is strictly
speaking a major API revision.

This partially, but not completely, addresses issue #40.

Now all constructors of SocketOption, Family, and SocketType will always
be declared, but using ones not supported on your system will lead to
runtime errors. To guard against these, new functions isSupportedFamily
etc. are introduced.

As a side effect, DummySocketOption__ goes away, so this is strictly
speaking a major API revision.

This partially, but not completely, addresses issue #40.
- Replace several error-exceptions with IO exceptions
- Add docs to SocketOption, getSocketOption, setSocketOption
- Mention isSupportedFamily in Family docs
- Insulate properly against socket option levels being undefined
- Remove export of packSocketType, intended for internal use but not
  actually used
@bmillwood
Copy link
Contributor Author

I hope the two commits I just added address your comments.

Worth mentioning: since I was switching packSocketType to a Maybe result anyway, I reasoned the API was breaking and we might as well just stop exporting it. It claims to be not for client use, only for the BSD module, but the BSD module doesn't appear to use it these days.

Probably the other "private" exports ought to be moved to an internal module.

@bmillwood
Copy link
Contributor Author

Hmm. Seems we already assume that SOL_SOCKET exists, and provide the sOL_SOCKET constant. I'm not sure why. Should I remove the tests for SOL_SOCKET (it almost certainly does exist if setsockopt does, I'd've thought) and/or remove the export of sOL_SOCKET?

@tibbe
Copy link
Member

tibbe commented Sep 8, 2012

I've merged a rebased version of these commits (had to fix an indentation error in a case statement that caused a compile error). I decided to keep packSocketType for now and make the decision whether to remove it until later. As for sOL_SOCKET constant, lets keep the current behavior for now.

These changes were pushed the develop branch, which will be the next major release.

@tibbe tibbe closed this Sep 8, 2012
@bmillwood
Copy link
Contributor Author

Okay, great, thanks.

(although the fact that you rebased them means that you kind of ended up committing code (even if only whitespace changes) under my name that I didn't write, and also means that the commits that exist in my repository are different from the ones in yours; couldn't you have done a merge instead?)

@tibbe
Copy link
Member

tibbe commented Sep 8, 2012

I can't merge code that doesn't compile as that will e.g. break future git
bisects. I added back packSocketType in a separate commit so that will be
in my name. I only made whitespace changes to your commit.

On Sat, Sep 8, 2012 at 1:08 PM, Ben Millwood notifications@github.comwrote:

Okay, great, thanks.

(although the fact that you rebased them means that you kind of ended up
committing code (even if only whitespace changes) under my name that I
didn't write, and also means that the commits that exist in my repository
are different from the ones in yours; couldn't you have done a merge
instead?)


Reply to this email directly or view it on GitHubhttps://github.com//pull/53#issuecomment-8397336.

@bmillwood
Copy link
Contributor Author

Okay, fair enough. I'd've been happy to go and fix the bit that didn't compile, though.

@tibbe
Copy link
Member

tibbe commented Sep 8, 2012

I'll let you do that next time. People's preferences differ in this area.
Some feel it's a pain to add up to another 24 hours of roundtrip time for
simple issues I can fix while commiting.

On Sat, Sep 8, 2012 at 4:11 PM, Ben Millwood notifications@github.comwrote:

Okay, fair enough. I'd've been happy to go and fix the bit that didn't
compile, though.


Reply to this email directly or view it on GitHubhttps://github.com//pull/53#issuecomment-8398950.

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.

None yet

2 participants