Remove write-stream from levelup!

[jcrugzz]

Thought I would go ahead and remove all the references to rip off the
bandaid. @rvagg let me know if i missed something but all the tests
pass. Closes #199

[rvagg]

cool, is there anything left in the functional-tests for this? does npm run-script alltests pass? I think there's still some fstream cruft in there that needs to be ripped out and once that's done perhaps there's nothing left in the functional tests so there's a bit more to clean up!

[jcrugzz]

@rvagg just took care of that. Everything is just npm test now

[rvagg]

"14 changed files with 1 addition and 1,068 deletions.", nice

Also needs WriteStream removed from the README, but you can leave that with me if you like because I also want to do some minor work on the streams wording to update it to streams2 anyway, unless you feel like having a go at that too.

The rest lgtm, this'll probably be v0.18.0 unless anyone has anything else to add before a release or has any objections to this PR?

This would also make @jcrugzz the 14th addition to the core contributors team (assuming he's happy with that), which is cool, he's been pretty active in promoting and talking about level* stuff around the place, including at the upcoming jsconf.co I believe. I've also heard word from inside Nodejitsu that the level* rumblings are becoming quite loud, which is great!

I need to do a write-up about the rationale behind removing this and the way forward with the level-packager packages for 0.18.0+.

It would be good if anyone with an alternative WriteStream implementation could get theirs ready for general release (@pgte, @maxogden, @brycebaril?). The test suite in level-ws could serve as a basic-functionality suite, it's not fully exported yet but we can do the same as what we have with AbstractLevelDOWN's suite so that other packages can import and run it. Also, the prototype for WriteStream is exported from level-ws so it's also available as a base if anyone wants to release a version with minor modifications, just take the parts of the prototype that you want.

[juliangruber]

I'm very strongly for bumping the major version on this one. Also see my suggestions on what tests not to remove.

[rvagg]

@juliangruber these are difficult tests to include without fstream compatibility, there's lots of additional LOC needed to perform the same job. Any takers on doing this?

Of course, level-fstream isn't a bad idea...

[juliangruber]

It seems to drop level.copy, level.createWriteStream and the fstream?

[rvagg]

@juliangruber that's correct, the fstream cruft was the biggest overhead in the current WriteStream impl and it's been flagged for removal for a while, unfortunately the functional tests all rely on it because they throw tar files around which can only be directly streamed to fstream-compatible streams.

[juliangruber]

to not lose tests I'm then for pulling in level-fs and level-fstream for now, until we've come up with tests that do the same but can be well written using only levelup core primitives.

[rvagg]

@jcrugzz I'm pretty sure concat-stream can finally be removed from the dependencies list with these changes too.

[jcrugzz]

@rvagg I got another half hour to kill before I crash so I'll take a stab at the docs. And sure I will take care of that.

@juliangruber I'd think that any tests that were removed would be replicated in the external module replacing its functionality. There is no need to pull anything extra into the levelup tests since it is a core lib (IMO).

[edit]: Also I would totally be ok being added as a core contributor :). I appreciate the consideration!

[rvagg]

The tests that @juliangruber identified are kind of important to ensure we don't have breakage across leveldb versions, but you're right that they probably don't belong here. We should probably at least have them in leveldown though so we don't lose it.

[juliangruber]

Oh yeah, leveldown is the better place for them.

[jcrugzz]

Agreed, leveldown sounds like the right place.

[mcollina]

I'm 👍 for hitting 1.0.0 on this.

However, people usually put too much emphasis on the "1.0" release (I saw a few complaining about node not being '1.0'), so we might want to include some more stuff in it. As an example, I also think we might want to put #201 in, too.

[rvagg]

I've just released MemDOWN@0.5.0 which supports the new ranges (gt/gte/lt/lte) as per dominic's tests, I've also extended AbstractLevelDOWN (@0.11.0) to include some help in supporting these.

There's a pull request for level.js that adds the test suite for the ranges and upgrades AbstractLevelDOWN but the implementation is required--if anyone feels like helping @maxogden on that I'm sure it would be appreciated.

I don't think I have this in LMDB yet but it's not very heavily used and it shouldn't be hard to add anyway.

IMO once level.js is done with the new ranges we can document them and deprecate start & end.

[juliangruber]

@mcollina we've had some discussions about the semver thing and as far as I remember this project uses the early chaotic version of semver, so breaking changes will happen before 1.0.0.

[rvagg]

fwiw my attitude towards semver is that the major version is uninteresting and irrelevant to any of my uses, except in the recent case where I took over the ssh package name and jumped straight to 1.0.0 to differentiate a completely different codebase from what was there previously. But even that was a fuzzy criteria and only done for the pageantry, as are all other major version bumps as far as I'm concerned.

I use "~x.y.z" for all my "dependencies" so x.y are important but it may as well just be y, there's no technical difference from what goes on in dependency resolution.

If y'all want a 1.0.0 then so be it, I care not, I'd be happy to keep on incrementing minor forevermore.

[jcrugzz]

A 1.0.0 may be fitting here especially if we toss in #201 and #208 since it does introduce core changes to the api. But when it comes down to it, 1.0.0 is just a marketing mechanism to instill stability in people's minds. I guess it depends if there are plans for more breaking changes or not if the plan has been to follow chaotic version of semver. Anyway its your call @rvagg if you want to get out the confetti for a 1.0.0 ;).

[dominictarr]

I think a good compromise here is to use semver just to indicate whether dependent modules may need to be rewritten (and this change will break lots of stuff), and indicate the stability of a project via separate documentation
(nodejs does this internally, and it's system is as good as any, http://nodejs.org/api/documentation.html#documentation_stability_index )

[juliangruber]

that's a good point, like level-sublevel currently doesn't support lt/lte/gt/gte and I'm pretty sure other modules don't either

[dominictarr]

adding lte?/gte? is a feature addition (normally minor), but removing writeStream is a breaking change (major)

[rvagg]

Just an update on this PR: we're needing a level-fstream (or even better level-write-fstream and level-read-fstream). I'm happy to do these as I've already done the necessary banging-of-head to understand how they work but anyone else is welcome to have a go before I manage to find the time to do it.

Once we have some fstream packages we can reimplement the tests that verify leveldb compatibility across versions (amongst other things that the fstream tests were verifying).

[dominictarr]

hmm, wasn't there some talk about removing fstream support?
is it just for the tests?

[rvagg]

ping @jcrugzz any status here or are you snowed under?

[jcrugzz]

@rvagg I had been but I can take some time this week. The big issue here was that the tests that I moved into level-fstream pass but technically shouldn't based on what we discussed in IRC a while back. I can't remember why but Ill dig back into it

[dominictarr]

I'm currently rewriting level-sublevel and intend to release level-sublevel@6 without writestreams,
it will support lte? gte? ranges and binary keys. I have bandwidth to work on this if someone can
indicate what needs to be done here.

[jcrugzz]

@dominictarr somehow the tests in level-fstream pass but for whatever reason they actually shouldn't. I cannot remember why they caused a false positive but @rvagg had pointed it out to me when I finished writing the module. Haven't had the time to redig into why unfortunately.

[ralphtheninja]

@jcrugzz I'm closing this PR and will continue working on your rebased branch (that lives in levelup now)