-
-
Notifications
You must be signed in to change notification settings - Fork 732
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
56f4fbb
commit 6c70b31
Showing
9 changed files
with
250 additions
and
234 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
language: node_js | ||
|
||
node_js: | ||
- 0.10 | ||
- 4.0 | ||
- 4 | ||
- 5 | ||
|
||
sudo: false |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,15 @@ | ||
'use strict'; | ||
|
||
// Load modules | ||
|
||
var Stringify = require('./stringify'); | ||
var Parse = require('./parse'); | ||
const Stringify = require('./stringify'); | ||
const Parse = require('./parse'); | ||
|
||
|
||
// Declare internals | ||
|
||
var internals = {}; | ||
const internals = {}; | ||
|
||
|
||
module.exports = { | ||
stringify: Stringify, | ||
parse: Parse | ||
}; | ||
exports.stringify = Stringify; | ||
exports.parse = Parse; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I missed the complaints thread, but still:
I get the consistency point, and I don't care about supporting node 0.10 either, but why is adding noise to the code base considered a good thing?
Does using
const
andlet
in the context of this module have any real benefit?Does using
const
andlet
overvar
makes this module easier to read and maintain?Arrow functions? Yes, they make the code easier to read.
6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistency is the point. all hapi core projects must follow the same linting and testing rules, and this module is no exception. this update makes this module adhere to the new rules.
6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'm the last person who's going to whine about your rules. Still I think completely banning the
var
keyword is just wrong. What possibly can go wrong in a 5 line function that you need twoconst
s there? Anyway, I like the ES6 features, but I don't think they are needed by default everywhere.6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var leaks outside of what people consider "normal" scope, let/const don't. It's all about intentions, with const it's obvious, with let you really place it where the scope of the variable should be, so no debate here for me, let will win in the end.
6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this module brakes all kind of npm usages. I'm using latest stable node (v 0.12.2) and I have "Use of const in strict mode." error
6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.12.12 is definitely not the latest stable node, the 5.x releases are stable and there's even a 4.x release that's in place for long term support
6c70b31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! "sudo n stable" gave me v5.0.0. Closed my issue #138