Skip to content
This repository was archived by the owner on Apr 24, 2020. It is now read-only.

Type may now be either a String or a Number. #175

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

Type may now be either a String or a Number. #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ function gcWrap (self) {}

function Socket(type) {
this.type = type;
this._zmq = new zmq.Socket(defaultContext(), types[type]);
this._type = typeof type === 'number' ? type : types[type];
Copy link
Collaborator

@ronkorving ronkorving Feb 14, 2013

Choose a reason for hiding this comment

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

so _type is the numerical version of type?
I reckon it makes sense to rename that a little bit to reflect that difference?

Copy link
Collaborator

@AlexeyKupershtokh AlexeyKupershtokh Apr 5, 2013

Choose a reason for hiding this comment

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

I think we need 2 properties and make this.type always be string, and this._type (I propose this.inttype) always be int.
Then we could make fill the both fields disregarding of what has been passed as the type argument.

// normalize this.inttype
this.inttype = typeof type === 'number' ? type : types[type];
// normalize this.type
Object.keys(types).forEach(function(type) {
  if (types[type] == this.inttype) { this.type = type; }
}, this);

Copy link
Author

@Schoonology Schoonology Apr 5, 2013

Choose a reason for hiding this comment

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

To be fair, I see most code using zmq just opting for the string version of Socket, and that might be a "nicer" method regardless.

In that case, I'd remove support for initializing from a Number entirely, opting to only use Socket(String).

Copy link
Collaborator

@AlexeyKupershtokh AlexeyKupershtokh Apr 7, 2013

Choose a reason for hiding this comment

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

@Schoonology actually according to http://api.zeromq.org/3-2:zmq-socket initializing socket with int seems more correct.

this._zmq = new zmq.Socket(defaultContext(), this._type);
this._zmq.onReady = this._flush.bind(this);
this._outgoing = [];
this._shouldFlush = true;
Expand Down
5 changes: 5 additions & 0 deletions test/test.socket.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ zmq.createSocket.should.equal(zmq.socket);
var sock = zmq.socket('req');
sock.type.should.equal('req');
sock.close.should.be.a('function');
sock.close();

sock = zmq.socket(zmq.ZMQ_REQ);
sock.type.should.equal(zmq.ZMQ_REQ);
sock.close.should.be.a('function');
Copy link
Collaborator

@ronkorving ronkorving Feb 14, 2013

Choose a reason for hiding this comment

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

Could you then also add a test to cross-check? Initialize with an integer, and compare against the string 'req', and vice versa?

Copy link
Collaborator

@AlexeyKupershtokh AlexeyKupershtokh Apr 5, 2013

Choose a reason for hiding this comment

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

@ronkorving this test will fail in this implementation.


// setsockopt

Expand Down