Skip to content
This repository was archived by the owner on Jan 7, 2022. It is now read-only.

exit if the user has requested a specific port#192

Closed
SvenDowideit wants to merge 2 commits intodat-ecosystem-archive:masterfrom
SvenDowideit:error-out-on-sepecifed-port
Closed

exit if the user has requested a specific port#192
SvenDowideit wants to merge 2 commits intodat-ecosystem-archive:masterfrom
SvenDowideit:error-out-on-sepecifed-port

Conversation

@SvenDowideit
Copy link
Copy Markdown

Signed-off-by: Sven Dowideit sven.dowideit@csiro.au

for dat-ecosystem/dat#945

Signed-off-by: Sven Dowideit <sven.dowideit@csiro.au>
@SvenDowideit SvenDowideit force-pushed the error-out-on-sepecifed-port branch from b9bdf27 to 4849deb Compare March 7, 2018 04:35
@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 7, 2018

Codecov Report

Merging #192 into master will decrease coverage by 0.45%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
- Coverage   84.86%   84.41%   -0.46%     
==========================================
  Files           7        7              
  Lines         304      308       +4     
==========================================
+ Hits          258      260       +2     
- Misses         46       48       +2
Impacted Files Coverage Δ
lib/network.js 90% <60%> (-10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85c1089...91503b0. Read the comment docs.

swarm.once('error', function () {
swarm.once('error', function (err) {
// Don't magically use any port if the users has requested a specific port
swarm.emit('errorport', err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can also we call the callback, cb?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

mmm, looking at that file - no idea, I don't know what it does, or how / when it should be called :/

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah, ok, that changes the if answer too - awesome, updating.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

mmm, looking at that file - no idea, I don't know what it does, or how / when it should be called :/

The callback can be used to know if there are any network connections (its called back after first round of discovery). But we can pass the error back as the first argument as well.

swarm.once('error', function (err) {
// Don't magically use any port if the users has requested a specific port
swarm.emit('errorport', err)
swarm.listen(0)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we call this if opts.port isn't set?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I originally added an if for that too - but the errorport handling shortcircuits before that, so it seemed cleaner to not add a duplicate if

@joehand
Copy link
Copy Markdown
Collaborator

joehand commented Mar 8, 2018

It'd be great to have a test for this.

Signed-off-by: Sven Dowideit <sven.dowideit@csiro.au>
swarm.once('error', function () {
swarm.once('error', function (err) {
// Don't magically use any port if the users has requested a specific port
swarm.emit('errorport', err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we use emit('error', new Error('specified port not available')) (or similar) for use cases outside the dat cli. Will require less documentation than a custom event =).

(Not sure if I left this before but not seeing it. Apologies if its duplicate.)

@joehand
Copy link
Copy Markdown
Collaborator

joehand commented Jul 5, 2018

I updated the CLI PR for this to avoid a custom error message. It'd still be good to have this in dat-node but will leave it to user for now.

@joehand joehand closed this Jul 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants