Skip to content

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.

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.

3 participants