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

Added disconnect method#14

Merged
juliangruber merged 3 commits intodat-ecosystem-archive:masterfrom
martinheidegger:disconnect
Jan 26, 2018
Merged

Added disconnect method#14
juliangruber merged 3 commits intodat-ecosystem-archive:masterfrom
martinheidegger:disconnect

Conversation

@martinheidegger
Copy link
Contributor

In order to properly shut down hyperdrives (Ctrl+C) it is good to have a disconnect method that disconnects all the archives (and networks) for a clean closing of an application. (Note: this is also quite useful for clean unit tests)

@codecov-io
Copy link

codecov-io commented Jan 24, 2018

Codecov Report

Merging #14 into master will increase coverage by 0.53%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   97.22%   97.75%   +0.53%     
==========================================
  Files           1        1              
  Lines          72       89      +17     
==========================================
+ Hits           70       87      +17     
  Misses          2        2
Impacted Files Coverage Δ
index.js 97.75% <100%> (+0.53%) ⬆️

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 cec9152...594dc31. Read the comment docs.

Copy link
Collaborator

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

otherwise, LGTM!

index.js Outdated
}

function create (data, cb) {
if (_disconnected) return cb(new Error('disconnected'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

those callbacks should be called after setImmediate(), a callback shouldn't be called in the same tick as its function is invoked

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 thought so too but on other places it also immediately returned (when in rome...). I took the liberty to fix all occurrences: c621c09


var store = toilet('state.json')
multidrive(store, createArchive, noop, function (err, drive) {
multidrive(store, createArchive, noopCb, function (err, drive) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this requirement to call the cb in closeArchive needs to be documented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I am a bit puzzled about this request. closeArchive was always supposed to call the callback (that didn't change) the only thing that changes is that closeArchive is now needed in some tests in order to disconnect the drives. The only way I found this necessary to be documented was in the readme: 594dc31

@martinheidegger
Copy link
Contributor Author

@juliangruber Thank you for the review. I pushed a few changes.

@juliangruber juliangruber merged commit 749afe5 into dat-ecosystem-archive:master Jan 26, 2018
@juliangruber
Copy link
Collaborator

Thanks Martin! Published as 5.2.0

@martinheidegger martinheidegger deleted the disconnect branch January 26, 2018 08:43
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