Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove write-stream from levelup! #207

Closed
wants to merge 6 commits into from
Closed

Conversation

jcrugzz
Copy link
Contributor

@jcrugzz jcrugzz commented Oct 14, 2013

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
Copy link
Member

rvagg commented Oct 14, 2013

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
Copy link
Contributor Author

jcrugzz commented Oct 14, 2013

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

@rvagg
Copy link
Member

rvagg commented Oct 14, 2013

"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
Copy link
Member

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

@rvagg
Copy link
Member

rvagg commented Oct 14, 2013

@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
Copy link
Member

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

@rvagg
Copy link
Member

rvagg commented Oct 14, 2013

@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
Copy link
Member

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
Copy link
Member

rvagg commented Oct 14, 2013

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

@jcrugzz
Copy link
Contributor Author

jcrugzz commented Oct 14, 2013

@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
Copy link
Member

rvagg commented Oct 14, 2013

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
Copy link
Member

Oh yeah, leveldown is the better place for them.

@jcrugzz
Copy link
Contributor Author

jcrugzz commented Oct 14, 2013

Agreed, leveldown sounds like the right place.

@mcollina
Copy link
Member

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
Copy link
Member

rvagg commented Oct 14, 2013

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
Copy link
Member

@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
Copy link
Member

rvagg commented Oct 14, 2013

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
Copy link
Contributor Author

jcrugzz commented Oct 14, 2013

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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

@rvagg
Copy link
Member

rvagg commented Nov 3, 2013

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
Copy link
Contributor

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

@rvagg rvagg mentioned this pull request Jan 25, 2014
@rvagg
Copy link
Member

rvagg commented Jun 24, 2014

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

@jcrugzz
Copy link
Contributor Author

jcrugzz commented Jun 24, 2014

@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
Copy link
Contributor

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
Copy link
Contributor Author

jcrugzz commented Jul 25, 2014

@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.

dominictarr referenced this pull request in dominictarr/multilevel Jul 25, 2014
@ralphtheninja
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bite the bullet and remove WriteStream completely?
6 participants