Remove unused escape characters and variables #59
Conversation
Allocating unused variables and using unused escape characters makes it hard to know which things actually require human attention. Some runtimes allocate memory for these variables, but I don't think that's a problem with Node.js.
| @@ -1,7 +1,7 @@ | |||
| module.exports = function (opts) { | |||
| return { | |||
| name: 'noauth', | |||
| create: function (_opts) { | |||
| create: function () { | |||
There was a problem hiding this comment.
The use of prefixed underscore is usually (e.g. often in TypeScript) for variables and arguments that you know you are not using, but would like to still explicitly name. For instance, for internal documentation, to indicate that this argument is provided and could be used, but is not used at the moment. So in this specific case I would recommend keeping it.
Long story short:
- If a name is not used => prefix it with underscore OR remove it
- If a name is used => do not prefix it with underscore
There was a problem hiding this comment.
I'm familiar with the convention, but I'm using eslint:recommended specifically to avoid any controversy about linter configuration. I think the right solution would be to write documentation for the plugin API rather than having to keep hinting at it with unused variables sprinkled around our code.
There was a problem hiding this comment.
If a name is not used => prefix it with underscore OR remove it
I think this is fine for personal code, but when working with others I think it's better to have a consistent rule rather than 'the tyranny of whoever touched it last'. I want to make two distinct points:
- We should defer 'code style' opinions to popular + conventional + conservative style guides (e.g. eslint:recommended).
- We should have proper documentation rather than abusing unused variables as load-bearing structures.
I want to make sure that you have an opportunity to respond, so I won't close my pull request now, but if this patch is unacceptable I think we should probably just close the pull request and discuss some other time.
There was a problem hiding this comment.
Well, ok, I won't make this a blocker to merge
| @@ -12,7 +12,7 @@ module.exports = function (opts) { | |||
| }) | |||
| } | |||
| }, | |||
| parse: function (str) { | |||
There was a problem hiding this comment.
I would actually prefix this one with underscore because it might be useful. It's more useful in the implementation than it is in tests. I.e. I agree with removing unused variables in tests.
| @@ -8,7 +8,7 @@ module.exports = function (opts) { | |||
| return { | |||
| name: 'onion', | |||
| scope: function() { return 'public' }, | |||
| parse: function (s) { return null } | |||
There was a problem hiding this comment.
I would keep this one, but use underscore
| @@ -87,7 +87,7 @@ module.exports = function (opts) { | |||
| port: port | |||
| } | |||
| }, | |||
| stringify: function (scope) { | |||
| stringify: function () { | |||
There was a problem hiding this comment.
I would keep this one, but use underscore
Allocating unused variables and using unused escape characters makes it
hard to know which things actually require human attention. Some
runtimes allocate memory for these variables, but I don't think that's a
problem with Node.js.