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

A few fixes #13

Closed
wants to merge 7 commits into from
Closed

A few fixes #13

wants to merge 7 commits into from

Conversation

coolaj86
Copy link
Contributor

A few fixes for #3 #9 #12 and also #11.

@coolaj86
Copy link
Contributor Author

In regards to e5f5373:

The assumption that you're receiving broadcasts to 239.255.255.250:1900 was incorrect.

If you want to extend this functionality in the future to do things other than M-SEARCH or have a dgram replacement that can listen in promiscuous mode, that would be better suited to go into another module rather than trying to do all of this here.

Furthermore, the search should not be limited to one result. Since you're searching multiple networks, you should get back an array.

This patch set is just for fixing bugs. I've started a rewrite that simplifies the code a lot and before I pull request that in as a solid 1.0 I'll probably allow multiple ssdp search results.

@@ -14,8 +14,8 @@ client.create = function create() {

function normalizeOptions(options) {
function toObject(addr) {
if (typeof addr === 'number') return { port: addr };
if (typeof addr === 'object') return addr;
if (typeof addr === 'number') { return { port: addr }; }
Copy link
Owner

Choose a reason for hiding this comment

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

Wow, please don't do this 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.

You mean add curly braces?

I've come across so many errors that result from not using braces when working with others. I'm a very very strict linter. If in doubt, spell it out.

That said, it's your code. I tried to make lint commits separate from code commits.

Copy link
Owner

Choose a reason for hiding this comment

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

This is quite consistent with node.js code style. Though, we usually put statement on a next line.

Anyway, it would be really great if this project will be using jscs for code style checking. I have suggested a configuration file in another comment, do you think it might be interesting for you to implement?

@indutny
Copy link
Owner

indutny commented Feb 11, 2015

@coolaj86 may I ask you to do it without style fixes for now? Or take .jscsrc from bn.js: https://github.com/indutny/bn.js/blob/master/.jscsrc ?

@coolaj86
Copy link
Contributor Author

How does one switch to using jscs? I installed it and I see the syntastic file exists, but where jshint gives hints every time I save in vim, jscs seems to do... nothing. Googling... was kinda hoping it would just be add a line to my vimrc or something.

Hints?

@coolaj86
Copy link
Contributor Author

Figured it out: https://coolaj86.com/articles/jscs-jscsrc-vim-syntastic-and-you/

Now going on to make rewrite my curly braces...

@coolaj86
Copy link
Contributor Author

jscs doesn't have an option for enforcing curly braces in the way you suggest (see jscs-dev/node-jscs#244) but I used grep to make sure I set everything back.

@indutny
Copy link
Owner

indutny commented Feb 11, 2015

Landed in ae7724a, thank you!

@indutny indutny closed this Feb 11, 2015
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.

2 participants