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

override _setupIteratorOptions, without clobbering ranges #45

Closed
wants to merge 1 commit into from

Conversation

dominictarr
Copy link

follows the patterns in #44 except it preserves ranges.

I'm a little wary of doing it this way, because _setupIteratorOptions is not an official public api of leveldown (not mentioned in the documentation). I've already been bitten by depending on obsecure edgecases in level, that have been removing in the course of routine maintainence. I would be more confidant about the stability of https://github.com/Level/encoding-down/pull/43/files because I know iterator isn't going anywere. But I can easily imagine someone forgetting about _setupIteratorOptions, refactoring it, and this then stops working.

@ralphtheninja
Copy link
Member

I'm a little wary of doing it this way, because _setupIteratorOptions is not an official public api of leveldown (not mentioned in the documentation)

We can make it part of the official public api.

I've already been bitten by depending on obsecure edgecases in level, that have been removing in the course of routine maintainence.

That's not good. What are the other types of problems you have been running in to?

I would be more confidant about the stability of https://github.com/Level/encoding-down/pull/43/files because I know iterator isn't going anywere.

Assuming it's part of the official api, this isn't that much different from implementing ._put(). There's also deferral of methods in deferred-leveldown that builds on this, which would break if overriding .iterator() (see https://github.com/Level/deferred-leveldown/blob/master/deferred-leveldown.js#L46-L50).

But I can easily imagine someone forgetting about _setupIteratorOptions, refactoring it, and this then stops working.

We write tests for this.

@dominictarr
Copy link
Author

sure if you want to make _setupIteratorOptions part of the official api, then i'm 👍 👍

Copy link
Member

@ralphtheninja ralphtheninja left a comment

Choose a reason for hiding this comment

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

LGTM

@ralphtheninja
Copy link
Member

@dominictarr How urgent is this? We can throw out a patch immediately if you like and fix the tests + docs later.

@ralphtheninja ralphtheninja mentioned this pull request May 18, 2018
@vweevers
Copy link
Member

vweevers commented May 18, 2018

This mutates the options. @dominictarr can you throw in a options = xtend(options)? With xtend.

Re: stability of _setupIteratorOptions: IMO this is a temporary solution anyway. It's not gonna cut it if you do db.createReadStream({ gt: null }) before the db has opened, or if you wrap the db with other stuff.

Down the line it should be solved in abstract-leveldown, possibly by reverting the null check and preparing implementations for that.

@dominictarr
Copy link
Author

@vweevers fixed to use xtend

@dominictarr
Copy link
Author

@ralphtheninja a patch soon would be helpful

@vweevers
Copy link
Member

fixed to use xtend

I don't see it. Did you push?

@ralphtheninja
Copy link
Member

4.0.1

@ralphtheninja
Copy link
Member

and in 5.0.1

@dominictarr
Copy link
Author

@ralphtheninja thanks! need a patch to level though, 3.0.1 still has leveldown@3, but master is leveldown@4

@vweevers
Copy link
Member

@dominictarr we'll get that out asap, we're also squeezing in other updates.

@ralphtheninja
Copy link
Member

@dominictarr Hmm I don't understand. Installing level gives you encoding-down@4.0.1 or do you need something else than that?

@dominictarr
Copy link
Author

@ralphtheninja sorry you are right! I see there is a patch version of level-packager!

@ralphtheninja
Copy link
Member

@ralphtheninja sorry you are right! I see there is a patch version of level-packager!

Cool. Let us know if there's anything else that's borked!

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.

3 participants