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

Implement Socket.socketpair to return Socket instances #4204

Merged
merged 5 commits into from Oct 5, 2016

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.

None yet

3 participants