Update README.md - Githubissues

ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging

https://github.com/ToothlessGear/node-gcm

Other

1.31k stars 206 forks source link

Update README.md #110

Closed Nonemoticoner closed 9 years ago

Nonemoticoner commented 9 years ago

I propose such solution for #70 No need to make it more complex. Let's keep it simple.

hypesystem commented 9 years ago

Great! Thanks for the PR. I have a few notes:

If you are only adding a single data field, the shorter syntax would be message.addData("key1", "msg1");
I would prefer to show off the Sender#send method, rather than Sender#sendNoRetry. All you need to do is change the method name -- it takes the same arguments.
I really do think the section should appear before usage. It seems the primary source for confusion is stumbling upon code, trying to run it, and seeing it not work. Let's make it easy to try to run something.

hypesystem commented 9 years ago

Uh, and one small thing: we should try and be consistent with the code style. The other examples use this for error checking/logging:

if(err) console.error(err);
else    console.log(result);

You should probably use this to be consistent :)

Nonemoticoner commented 9 years ago

Done, as you pleased. I'm not that convinced to be so consistent. For me, it implies too much Ctrl+C > Ctrl+V bad practice but whatever. I believe you can merge it now.

hypesystem commented 9 years ago

Great! Remember to add yourself to the contributors list in package.json and in the README - then I'll merge :)

Just a note on the consistency of code style (that you comment on): the code people first see, using the library, will probably use the style that they will themselves adopt. I think it's significant what we show new users of the library. Does it have to be this particular convention? No. But showing them that we stick to a convention fosters a better mentality (and more readable code).

Nonemoticoner commented 9 years ago

Done, you can merge ;)

hypesystem commented 9 years ago

Sorry for the long wait - I got hit by the flu :(