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

1.0.0-wip #289

Merged
merged 19 commits into from
Mar 17, 2015
Merged

1.0.0-wip #289

merged 19 commits into from
Mar 17, 2015

Conversation

kesla
Copy link
Contributor

@kesla kesla commented Oct 19, 2014

Let's get a 1.0.0 done to compliment [email protected]

Have I missed something else we should try and get into this release?

@dominictarr
Copy link
Contributor

👍

1 similar comment
@juliangruber
Copy link
Member

👍

@rvagg
Copy link
Member

rvagg commented Oct 19, 2014

the blocker for removing writestream was replacing the functional tests that involved fstream, it seems that both myself and @jcrugzz have been a little too busy to clean this out, but there's a starting point here if you want to pick up the effort: https://github.com/Level/level-fstream

Alternatives are: just ignore the functional tests for now and make a TODO issue to get it done in the future (risky), or don't remove writestream at all and do it later instead (also risky, it'd be nice to wean people off a bundled writestream IMO). Thoughts?

@mcollina
Copy link
Member

Will writestream be part of the level package?

Can we bless a writestream impl and put it inside devDependencies?

@rvagg
Copy link
Member

rvagg commented Oct 20, 2014

open for discussion, I don't think we resolved this, I'd like to hear from writestream implementers on their opinions though--the current state of things suggests that it's difficult to come up with a single implementation that is generically good and that their performance is heavily usage dependent

@juliangruber
Copy link
Member

this might be old news, but what if we have the simplest implementation in levelup but advice to use others for heavier needs?

@mcollina
Copy link
Member

I really think we should put the simplest writestream imp in devDependencies, something like https://www.npmjs.org/package/level-write-stream, and use that for tests.

@Raynos
Copy link
Member

Raynos commented Oct 20, 2014

something like level-write-stream but not level-write-stream as it needs way more love & tests and stuff. I'd basically need rewriting.

@kesla
Copy link
Contributor Author

kesla commented Nov 14, 2014

Hey! Let's get this discussion going again.

@rvagg:

Alternatives are: just ignore the functional tests for now and make a TODO issue to get it done in the future (risky), or don't remove writestream at all and do it later instead (also risky, it'd be nice to wean people off a bundled writestream IMO). Thoughts?

I say that we take this and release it as levelup 1.0 - keeping write-stream for now. TBH I don't see the harm in, as @juliangruber suggested, keeping a simple write-stream in core. I remember that we agreed to remove write-stream, but I don't really remember why :)

@rvagg
Copy link
Member

rvagg commented Nov 15, 2014

Yep, given the stall, I'm good with just getting something out, so let's bail on writestream removal.

@kesla we just need to get the functional tests passing some form for me to be 👍 on this.

@jcrugzz
Copy link
Contributor

jcrugzz commented Nov 15, 2014

Being the one who removed the write-stream, Im still indifferent on it. It makes sense to remove it if we are going to point people to proper write-stream implementations. I believe the argument here was that if there was no good write-stream for most use cases, so we should educate people on implementations for various use cases so we aren't getting bugs on an implementation just used for testing mostly.

Either way I have rebased my remove-ws branch to reflect the current state of master @kesla. What I'm still unsure of is how the level-fstream tests pass when they should be broken. It leads me to believe that they must be broken in master as well since the tests and implementation were copied straight out of levelup. @rvagg any IRC logs about that time we discussed this in ##leveldb?

@rvagg
Copy link
Member

rvagg commented Nov 15, 2014

@jcrugzz no idea sorry, removing writestream is still a worthy goal and we reached an acceptable level of consensus to make it happen, it's just holding us back so let's go with an easier 1.0 and aim for a 2.0 that removes writestream. Sound good?

@jcrugzz
Copy link
Contributor

jcrugzz commented Nov 15, 2014

@rvagg yea I have no qualms about it.

@mcollina
Copy link
Member

How about moving the current writestream to a separate module, so it can be
reused by sublevel? Also, it can be easily removed later on (we might even
emit a deprecation warning).
Il dom 16 nov 2014 alle 00:42 Jarrett Cruger [email protected] ha
scritto:

@rvagg https://github.com/rvagg yea I have no qualms about it.


Reply to this email directly or view it on GitHub
#289 (comment).

@ralphtheninja
Copy link
Member

Ok, so now four more months has passed. Lets give this a try again :)

I think we all want to get 1.0.0 out the door. If I understood this correctly, the question is rather if we want to keep write stream or not.

Releasing 1.0.0 imo has higher priority than throwing out the write stream and is a decision we could push into the future.

Once 1.0.0 is out, we can decide to deprecate the write stream and release 1.1.0 and then release 2.0.0 without the write stream but with tests showing a canonical example.

@juliangruber
Copy link
Member

what if we really just leave the simplest api compatible version of a writeStream in core, so it still works but people who require faster performance are advised to use another module.

something like this:

db.prototype.createWriteStream = function(){
  var self = this;
  var w = Writable();
  w._write = function(kv, _, done){
    var method = kv.type || 'put';
    db.put(kv.key, kv.value, done);
  }
  return w;
}

@ralphtheninja
Copy link
Member

sgtm, we should probably do something while we're releasing 1.0.0

@ralphtheninja
Copy link
Member

@jcrugzz 👍 I'm rebasing your branch onto 1.0.0-wip and pushing up to this repo for now. With your branch we don't have to wait for level-ws to be merged. So up to the community what we want. @rvagg 's branch with level-ws tests or @jcrugzz 's branch with no level-ws tests :)

https://github.com/rvagg/node-levelup/tree/remove-ws

@mcollina
Copy link
Member

no level-ws tests for me, and leave ws to the community. We need a good
README explaining this removal, though.
Il giorno mar 10 mar 2015 alle 17:28 Lars-Magnus Skog <
[email protected]> ha scritto:

@jcrugzz https://github.com/jcrugzz [image: 👍] I'm rebasing your
branch onto 1.0.0-wip and pushing up to this repo for now. With your
branch we don't have to wait for level-ws to be merged. So up to the
community what we want. @rvagg https://github.com/rvagg 's branch with
level-ws tests or @jcrugzz https://github.com/jcrugzz 's branch with no
level-ws tests :)


Reply to this email directly or view it on GitHub
#289 (comment).

@ralphtheninja
Copy link
Member

@jcrugzz Mind adding some some to this section?

https://github.com/rvagg/node-levelup/tree/remove-ws#what-happened-to-dbcreatewritestream

There are a few typos (Disclaimper) and a todo comment.

@rvagg
Copy link
Member

rvagg commented Mar 10, 2015

fwiw I'm onsite with a client this week (and last) but plan on dedicating next week to some of my open source backlog and this is near the top of my list, unfortunately I can't afford the headspace to get stuck in here until then but you're welcome to hold me to getting to this next week!

@ralphtheninja
Copy link
Member

@rvagg Hopefully you'll only need to publish when you come back :)

@ralphtheninja
Copy link
Member

I merged master into 1.0.0-wip so merging this PR should be trivial.

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 13, 2015

Big +1 to this PR and @ralphtheninja !

@juliangruber
Copy link
Member

I'm merging now since there's so many +1s and this project desparately needs a new version

juliangruber added a commit that referenced this pull request Mar 17, 2015
@juliangruber juliangruber merged commit 79787f6 into master Mar 17, 2015
@rvagg rvagg removed the in progress label Mar 17, 2015
@ralphtheninja
Copy link
Member

👯

@mcollina
Copy link
Member

So 🍻 for everybody. It's St. Patrick's day too!! 👯

@mcollina mcollina deleted the 1.0.0-wip branch March 17, 2015 17:52
@mcollina
Copy link
Member

@rvagg it's your turn to publish this goodness, so we can all start updating our modules!

@kesla
Copy link
Contributor Author

kesla commented Mar 17, 2015

BOOM! 👍

@mattstyles
Copy link

Not really the place to plaudits but... 🍻 👏

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.