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.

Sign up for GitHub

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

Remove unused escape characters and variables #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove unused escape characters and variables #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ module.exports = function (plugs, wrap) {
// If a callback is not registered to be called back when the servers are
// fully started, our default behaviour is just to print any errors starting
// the servers to the log
startedCb = (err, result) => {
startedCb = (err) => {
if (err) {
console.error("Error starting multiserver server: " + err)
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ module.exports = ({ scope = 'device', host, port, external, allowHalfOpen, pause
}

// Remove IPv6 scopeid suffix, if any, e.g. `%wlan0`
resultHost = resultHost.replace(/(\%\w+)$/, '')
resultHost = resultHost.replace(/(%\w+)$/, '')

return toAddress(resultHost, port)
}
Expand Down
4 changes: 2 additions & 2 deletions plugins/noauth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module.exports = function (opts) {
return {
name: 'noauth',
create: function (_opts) {
create: function () {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@christianbundy christianbundy Sep 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@staltz staltz Sep 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Well, ok, I won't make this a blocker to merge

return function (stream, cb) {
cb(null, {
remote: opts.keys.publicKey,
Expand All @@ -12,7 +12,7 @@ module.exports = function (opts) {
})
}
},
parse: function (str) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

parse: function () {
return {}
},
stringify: function () {
Expand Down
6 changes: 3 additions & 3 deletions plugins/onion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = function (opts) {
return {
name: 'onion',
scope: function() { return 'public' },
parse: function (s) { return null }
Copy link
Member

Choose a reason for hiding this comment

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

I would keep this one, but use underscore

parse: function () { return null }
}
}

Expand Down Expand Up @@ -63,7 +63,7 @@ module.exports = function (opts) {
}
}

tryConnect(connectOpts(daemonProxyOpts), function(err) {
tryConnect(connectOpts(daemonProxyOpts), function() {
tryConnect(connectOpts(browserProxyOpts), function(err) {
cb(err)
})
Expand All @@ -87,7 +87,7 @@ module.exports = function (opts) {
port: port
}
},
stringify: function (scope) {
stringify: function () {
Copy link
Member

Choose a reason for hiding this comment

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

I would keep this one, but use underscore

return null
}
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/ws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ module.exports = function (opts = {}) {
},
parse: function (str) {
var addr = URL.parse(str)
if(!/^wss?\:$/.test(addr.protocol)) return null
if(!/^wss?:$/.test(addr.protocol)) return null
return addr
}
}
Expand Down
13 changes: 6 additions & 7 deletions test/plugs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
var tape = require('tape')
var pull = require('pull-stream')
var Pushable = require('pull-pushable')
const fs = require('fs')

var Compose = require('../compose')
var Net = require('../plugins/net')
Expand Down Expand Up @@ -165,7 +164,7 @@ tape('net: do not listen on all addresses', function (t) {

var addr = fake_combined.stringify('local') // returns external
console.log('addr local scope', addr)
combined.client(addr, function (err, stream) {
combined.client(addr, function (err) {
t.ok(err, 'should only listen on localhost')
close(function() {t.end()})
})
Expand Down Expand Up @@ -247,7 +246,7 @@ tape('error if try to connect on wrong protocol', function (t) {

t.equal(combined_ws.parse(combined.stringify()), null)

combined_ws.client(combined.stringify(), function (err, stream) {
combined_ws.client(combined.stringify(), function (err) {
t.ok(err)
t.end()
})
Expand Down Expand Up @@ -384,7 +383,7 @@ function testAbort (name, combined) {
throw new Error('should never happen')
})

var abort = combined.client(combined.stringify(), function (err, stream) {
var abort = combined.client(combined.stringify(), function (err) {
t.ok(err)

// NOTE: without the timeout, we try to close the server
Expand All @@ -408,10 +407,10 @@ testAbort('combined.ws', combined_ws)

tape('error should have client address on it', function (t) {
// return t.end()
check = function (id, cb) {
check = function () {
throw new Error('should never happen')
}
var close = combined.server(function (stream) {
var close = combined.server(function () {
throw new Error('should never happen')
}, function (err) {
t.ok(/^net:/.test(err.address))
Expand All @@ -422,7 +421,7 @@ tape('error should have client address on it', function (t) {

//very unlikely this is the address, which will give a wrong number at the server.
var addr = combined.stringify().replace(/shs:......../, 'shs:XXXXXXXX')
combined.client(addr, function (err, stream) {
combined.client(addr, function (err) {
//client should see client auth rejected
t.ok(err)
console.log('Calling close')
Expand Down