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

use strict #785

Merged
merged 6 commits into from
Aug 6, 2015
Merged

use strict #785

merged 6 commits into from
Aug 6, 2015

Conversation

BridgeAR
Copy link
Contributor

This is going to improve the speed and code safety a tiny bit by using use strict plus some style changes (adding semicolons). I added the later as I read that most of you seemed to prefer those. Using use strict in node should not break any code if it has not been broken before, since node encapsulates each file and does not apply it global.

// var msgpack = {
// encode: MsgPack.pack,
// decode: function(str) { return MsgPack.unpack(new Buffer(str)); }
// };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain to someone not so familiar with the codebase why you commented this out? If it's not used at all, it should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

module.exports = msgpack; is already commented out and therefor this is a unused variable. I was not sure what this part was for and normally I'd just delete unused code, but I'd prefer to know the reason for the original code before deleting it :).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Looks to me like they were considering using MessagePack for this functionality, but as the node library is slower than native JSON encoding and this is for benchmarking only (so compression gains were not useful), the decision was made just to use JSON instead. I think this is safe to be deleted.

@erinishimoticha
Copy link
Contributor

I fully support this! I asked one question about some commented code.

Ruben Bridgewater added 6 commits July 22, 2015 17:50
This is going to improve the performance minimal and improves the safety of the code
This is just a style change
util.print has been deprecated in node
@BridgeAR
Copy link
Contributor Author

Rebased

@josephpage
Copy link

👍

erinishimoticha added a commit that referenced this pull request Aug 6, 2015
Add "use strict", semicolons, whitespace & code cleanup, remove util.print.
@erinishimoticha erinishimoticha merged commit 6cae0b8 into redis:master Aug 6, 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.

None yet

3 participants