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

optional location & options arguments #208

Merged
merged 1 commit into from
Nov 3, 2013
Merged

optional location & options arguments #208

merged 1 commit into from
Nov 3, 2013

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 14, 2013

ref #206 as per @mmalecki's suggestion

This gives two new constructor forms: levelup(options[, callback ]) and levelup(db[, callback ]).

If you don't provide a location argument but do provide an 'object' as your first argument then it must have a 'db' property that's a 'function' which is assumed to be a backend factory; otherwise it fails. The second form is where you don't provide a location argument but you do provide a 'function' as your first argument, this is assumed to be a back-end factory. In both cases a second argument is allowable, a callback 'function' but this is optional. We can't do a form where you provide a location as the first argument and a back-end factory as the second because we can't tell if it's supposed to be a factory or a callback, unless you provide a callback as well, but then, you're getting verbose anyway so it's just silly.

var levelup = require('levelup')
var memdown = require('memdown')

var db = levelup({ db: memdown })

// or

var db = levelup(memdown) 

required new DeferredLevelDOWN and MemDOWN releases to make the location optional.

@juliangruber
Copy link
Member

+1

@mmalecki
Copy link

Why a factory and not just an instance?

@juliangruber
Copy link
Member

@mmalecki because you can close and open a database. It even says in the readme that reopening a database is not adviced, so I'm inclined to drop that feature and pass in an instance here, way cleaner.

@ralphtheninja
Copy link
Member

+1 for instance

@dominictarr
Copy link
Contributor

so, when this is a leveldown instance, should it be preopened?
or will levelup call db.open?

@rvagg
Copy link
Member Author

rvagg commented Oct 16, 2013

if it's not a factory then it'd need separate options for setting up the instance and setting up levelup

var db = levelup({ valueEncoding: 'json' }, leveldown({ createIfMissing: false }))

too messy IMO, I'm keen to stick with the factory.

Letting LevelUP manage instantiation gives it a chance to properly manage state for deferred-open operations, it knows exactly what state it's up to, it can use the callback to leveldown to trigger a completion to the open.

+1 to keeping factory, -1 to passing in an instance

@dominictarr
Copy link
Contributor

an instance could be okay, if you have already opened the instance.
but otherwise, I think @rvagg has a fair point. We don't want to add more edge cases.

@mmalecki
Copy link

After reconsidering the open case, I'm +1 on factory. PR LGTM.

@juliangruber
Copy link
Member

+1 for merging in, then MemDB can just be require('level-packager')(require('memdown')) and not have to provide a dummy database location.

rvagg added a commit that referenced this pull request Nov 3, 2013
optional location & options arguments
@rvagg rvagg merged commit 386f3cd into master Nov 3, 2013
@rvagg rvagg deleted the optinitargs branch November 3, 2013 03:28
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.

5 participants