-
Notifications
You must be signed in to change notification settings - Fork 32
Conversation
Hi Gerald, very cool that you are doing continued work on this! 👍 |
Thanks Holger! |
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.
Just to re-iterate @holgerd77's comment, we love that you are continuing to contribute! Thank you :)
I made a couple minor suggestions to simplify the code. But also, I noticed a lot of what looks like lint fixes. Were these generated using lint:fix
? If so, you should do any style/whitespace fixes in a separate PR so it makes code reviews easier. And a related question: if lint:fix
is making these changes, why is it changing code that is already passing lint checks?
lib/rpc/modules/net.js
Outdated
|
||
class Net { | ||
constructor (node) { | ||
const service = node.services.find(s => s.name === 'eth') | ||
this._chain = service.chain | ||
this._node = node | ||
this._peerPool = service.synchronizer.pool |
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 can just grab the pool directly from service:
this._peerPool = service.synchronizer.pool | |
this._peerPool = service.pool |
test/rpc/net/peerCount.js
Outdated
{ | ||
name: 'eth', | ||
chain: chain, | ||
synchronizer: { |
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.
Same here. Suggested change:
services: [
{
name: 'eth',
chain: chain,
pool: { peers: [1, 2, 3] }
}
I think it's the default Prettier config in my VS Code code setup doing that. I'll refactor and change my editor settings to abide by the Standard config. |
Changes to the non- |
Should be ready now. |
Looks like there are still a lot of prettier generated changes, but they're limited to the test scripts so I'll go ahead and merge this PR. Would be ideal in future PRs to limit diffs to just the relevant code changes. Regardless, thanks again for this contribution! 👍 |
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.
LGTM
No description provided.