Implement Socket.socketpair to return Socket instances #4204

Merged
merged 5 commits into from Oct 5, 2016

Projects

None yet

3 participants

@etehtsea
Contributor
etehtsea commented Oct 5, 2016

No description provided.

@headius

Functionality changes look fine, but @eregon needs to weigh in on the ruby/spec change.

+
+ s1.should be_an_instance_of(Socket)
+ s2.should be_an_instance_of(Socket)
+ end
@headius
headius Oct 5, 2016 Member

@etehtsea We usually update from ruby/spec via @eregon's process of migrating subtree commits back and forth. I'm not sure, but this separate commit may interfere with that process.

@eregon
eregon Oct 5, 2016 Member

It's OK to add specs directly here, even if the commits contains other changes. The advantages is it's tested directly against jruby and we don't need to wait for a spec merge.
However I prefer if the specs are in a commit alone as usually it means I don't need to edit the commit message 😃

@headius headius referenced this pull request in ruby/spec Oct 5, 2016
Closed

Improve Socket#socketpair specs #303

+ s1, s2 = Socket.public_send(@method, :UNIX, :STREAM)
+
+ s1.should be_an_instance_of(Socket)
+ s2.should be_an_instance_of(Socket)
@eregon
eregon Oct 5, 2016 Member

The opened file descriptors should be closed here.

@etehtsea
etehtsea Oct 5, 2016 Contributor

done

@etehtsea
Contributor
etehtsea commented Oct 5, 2016

Done.

etehtsea added some commits Oct 5, 2016
@etehtsea etehtsea Fix Socket#socketpair
sock2 was left uninitialized
cb3a35d
@etehtsea etehtsea Add Socket#socketpair response type spec 2ffd857
@etehtsea etehtsea Fix Socket#socketpair shared spec
303e5d8
@headius headius merged commit 4642234 into jruby:master Oct 5, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@headius headius added this to the JRuby 9.1.6.0 milestone Oct 5, 2016
@etehtsea etehtsea deleted the etehtsea:socket-socketpair branch Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment