add destroy method

[calvinmetcalf]

based on discussion at Level/level-js#29 a standardized destroy method on the prototype is a lot more portable then the current thing leveldown has on the constructor, especially as it's easy to call a constructor's destroy method from a prototype method then the other way around.

[dominictarr]

sounds reasonable. +1

[mcollina]

👍

[rvagg]

👎 for having destroy() in here and 👎 in particular for having it on the prototype. This is backend-specific behaviour and IMO you should be calling the backend's specific destroy. In LevelDB for instance it's not an instance method, it's a static that you provide a path to so it's going to be tricky to set it up nicely. We removed destroy() from LevelUP but you can still leveldown.destroy(path)

[calvinmetcalf]

the idea being to abstract the back end specific behavior it is easier for an instanced method to call a static one then the other way around, leveldown could do leveldown.prototype.destroy = function(callback) {leveldown.destroy(this.path,callback);} but calling an instanced method from a static is much trickier meaning its harder to write a drop in replacement adapter that has an instance style destroy.

[rvagg]

it'll also have to check open status before destroying, having it on the prototype will make the API consumer assume that it has control over the whole instance so it hast to act that way too.

Any more votes on this? I'm still not convinced but will go with the flow if y'all think this is a good idea.

[mcollina]

I'm ok with a common way of destroying/removing a database. It benefits all
of us that use multiple backends, simplifying testing and so on. I don't
have any preference on how this is done, either on the instance or as a
static function that is easily fetchable from a LevelUp instance. It's not
going to be a highly used function, so we can design it from an
implementation perspective. What is easier in LevelDOWN? Changing/coding
C++ is more time-consuming than JS, so let's give it preference.

[dominictarr]

wouldn't the destroyer need to know about the specific way the back end is implemented? the *down seems like the the right place for it because that is where the knowledge about the specifics of the backend is located.

[rvagg]

I think this is more a question of whether this should be a static method off the *down implementations (not necessarily something for abstract-leveldown to care about?) or a member of the *down implementation prototype.

In LevelDB, the destroy() just needs to be static because you pass it a path and it does its thing, no need to know about a particular instance, it's really just a directory cleaning mechanism. It could be on the prototype in which case we'd already have the location so it wouldn't need additional arguments but the catch is that the db would need to be in a closed state before it was destroyed. Or would destroy() call close() as a first step?

I can't think what this would look like for LMDB, I'm not sure it has a native destroy(), I'd be tempted to rimraf() but that might not be such a good idea.

MemDOWN is just a matter of GC, nothing concrete needed there except perhaps resetting the arrays and storage objects.

Level.js is where this discussion originated, I have no idea what's required on that front or what makes sense from an implementation perspective.

The biggest question for contributors here to answer is related to is this a good API?

LevelDOWN#destroy(callback) and possibly LevelUP#destroy([callback]). Or just leave it up to all of the separate *down implementations to do it their own way and expose their own API for the user to reach in to?

[heapwolf]

Fwiw I just rmrf and I'm happy. Is there some advantage I'm not seeing?

On Friday, March 21, 2014, Rod Vagg notifications@github.com wrote:

I think this is more a question of whether this should be a static method
off the *down implementations (not necessarily something for
abstract-leveldown to care about?) or a member of the *down implementation
prototype.

In LevelDB, the destroy() just needs to be static because you pass it a
path and it does its thing, no need to know about a particular instance,
it's really just a directory cleaning mechanism. It could be on the
prototype in which case we'd already have the location so it wouldn't need
additional arguments but the catch is that the db would need to be in a
closed state before it was destroyed. Or would destroy() call close() as
a first step?

I can't think what this would look like for LMDB, I'm not sure it has a
native destroy(), I'd be tempted to rimraf() but that might not be such a
good idea.

MemDOWN is just a matter of GC, nothing concrete needed there except
perhaps resetting the arrays and storage objects.

Level.js is where this discussion originated, I have no idea what's
required on that front or what makes sense from an implementation
perspective.

The biggest question for contributors here to answer is related to is this
a good API?

LevelDOWN#destroy(callback) and possibly LevelUP#destroy([callback]). Or
just leave it up to all of the separate *down implementations to do it
their own way and expose their own API for the user to reach in to?

Reply to this email directly or view it on GitHubhttps://github.com//pull/24#issuecomment-38338717
.

Paolo Fragomeni
Founder, Here is How
http://hereishow.to

github.com/hij1nx
twitter.com/hij1nx

[rvagg]

@hij1nx it only deletes LevelDB files so if there's other cruft in there then it won't remove that and will leave the directory intact (otherwise the directory goes too)

[heapwolf]

Ah, right! Maybe a use case could be where you have metadata
(level-manifest)?

On Friday, March 21, 2014, Rod Vagg notifications@github.com wrote:

@hij1nx https://github.com/hij1nx it only deletes LevelDB files so if
there's other cruft in there then it won't remove that and will leave the
directory intact (otherwise the directory goes too)

Reply to this email directly or view it on GitHubhttps://github.com//pull/24#issuecomment-38340747
.

Paolo Fragomeni
Founder, Here is How
http://hereishow.to

github.com/hij1nx
twitter.com/hij1nx

[dominictarr]

in the case of wrappers around other databases it may need to send a protocol message, etc,
so it's coupled to the specific backend - may not always be as simple as deleting some files.

[calvinmetcalf]

checking in on this, do we like it, do we think it needs improvement, is it being blocked by leveldown?

[rvagg]

I'm having difficulty forming a strong enough opinion about this! As suggested in Level/leveldown#97, I'm warming to the idea.

[calvinmetcalf]

ping now that Level/leveldown#97 is merged?

[rvagg]

ummmmmm maybe, anybody else?

[ghost]

Yeah, 👍

[vweevers]

I'm having difficulty pinpointing when and where the decision was made, but methods like destroy are leveldown-specific and cannot be supported in all implementations. As such, they don't belong in abstract-leveldown.

IMO a more generic solution would be a method like clear, which would empty a db while in use. This can have a default implementation: iterating and deleting all keys. Different implementations can optimize this in their own way.

Such a method would differ from leveldown#destroy, which also deletes the parent folder and assumes the db is closed (which is why the method is not on the prototype).

@ralphtheninja I vote for closing this and #42 and to open a new issue for clear. WDYT?

[ralphtheninja]

@vweevers SGTM!