Skip to content

Implement Socket.socketpair to return Socket instances#4204

Merged
headius merged 5 commits intojruby:masterfrom
etehtsea:socket-socketpair
Oct 5, 2016
Merged

Implement Socket.socketpair to return Socket instances#4204
headius merged 5 commits intojruby:masterfrom
etehtsea:socket-socketpair

Conversation

@etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Oct 5, 2016

No description provided.

Copy link
Member

@headius headius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😃

s1, s2 = Socket.public_send(@method, :UNIX, :STREAM)

s1.should be_an_instance_of(Socket)
s2.should be_an_instance_of(Socket)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opened file descriptors should be closed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@etehtsea etehtsea force-pushed the socket-socketpair branch 2 times, most recently from b0fffa7 to 88f82fd Compare October 5, 2016 15:12
@etehtsea
Copy link
Contributor Author

etehtsea commented Oct 5, 2016

Done.

@headius headius merged commit 4642234 into jruby:master Oct 5, 2016
@headius headius added this to the JRuby 9.1.6.0 milestone Oct 5, 2016
@etehtsea etehtsea deleted the socket-socketpair branch October 5, 2016 15:39
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.

3 participants