Conversation
8050c5b to
d7b2bf2
Compare
manishrjain
left a comment
There was a problem hiding this comment.
High level comments. You could still keep the current badger.Open(opts), but just change how opts is being created to achieve the same intended effect.
opts := badger.DefaultOptions().SetDir("dir").SetValueDir("vdir").SetReadOnly().SetSomething(...)
badger.Open(opts)
It could be further enhanced in various ways to cut down some boilerplate.
Reviewable status: 0 of 36 files reviewed, all discussions resolved (waiting on @ashish-goswami and @manishrjain)
|
I thought about having a builder API, I'm not against it but it didn't look as nice because you still had to somehow have the My biggest concern is having the path we need to open a file as part of the options instead of a parameter. That single change makes the code much easier (most users use the same path for both the tables and values dirs) |
|
Yeah, I've been thinking about it and while I like the fact that having a Currently: opts := badger.DefaultOptions
opts.Dir = "mydir"
opts.ValueDir = "mydir"
db, err := badger.Open(opts)Functional Options API: db, err := badger.Open("mydir")Builder API: db, err := badger.Open("mydir", badger.DefaultOptions())Alternatively, we could receive a pointer and assume that a nil value corresponds to the default options, but not sure whether that's easier to understand than the Functional Options API |
|
After a conversation with @manishrjain, I will rework on this API to keep it backwards compatible with the current one and instead offer a more convenient way to create The biggest change will be the creation of a db, err := badger.Open(options.Default(path))`We will also provide a builder API on db, err := badger.Open(options.Default(path).WithReadOnly(true))As this change is now backwards compatible it can be removed from the v2.0.0 milestone. |
|
As agreed with @manishrjain, I updated the PR to provide a change that will be backwards compatible with I did decide it was probably better not to be backwards compatible with The output of go doc is much nicer than before too. And the documentation for each option is now on WithX. Please review. |
martinmr
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @ashish-goswami, @campoy, and @golangcibot)
options.go, line 151 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 104 characters (from
lll)
Hard to tell in reviewable if these warnings have been fixed but they are marked as outdated in github. Just go through them once more to make sure.
options.go, line 130 at r3 (raw file):
// WithDir returns a new Options value with Dir set to the given value. // The original Options value is never modified.
How is this statement true? You are changing the value of opt.Dir and then returning the same opt value. Isn't that the opposite of what this sentence says? Or does setting the function signature to func (opt Options) instead of func (opt *Options) means the copy is implicit?
campoy
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @ashish-goswami, @golangcibot, and @martinmr)
options.go, line 151 at r2 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
Hard to tell in reviewable if these warnings have been fixed but they are marked as outdated in github. Just go through them once more to make sure.
Done.
options.go, line 162 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 116 characters (from
lll)
Done.
options.go, line 172 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 122 characters (from
lll)
Done.
options.go, line 175 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 101 characters (from
lll)
Done.
options.go, line 182 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 118 characters (from
lll)
Done.
options.go, line 226 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 108 characters (from
lll)
Done.
options.go, line 236 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 122 characters (from
lll)
Done.
options.go, line 240 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 101 characters (from
lll)
Done.
options.go, line 247 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 102 characters (from
lll)
Done.
options.go, line 257 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 112 characters (from
lll)
Done.
options.go, line 260 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 105 characters (from
lll)
Done.
options.go, line 268 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 108 characters (from
lll)
Done.
options.go, line 278 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 120 characters (from
lll)
Done.
options.go, line 288 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 130 characters (from
lll)
Done.
options.go, line 291 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 128 characters (from
lll)
Done.
options.go, line 298 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 108 characters (from
lll)
Done.
options.go, line 308 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 116 characters (from
lll)
Done.
options.go, line 318 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 120 characters (from
lll)
Done.
options.go, line 322 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 101 characters (from
lll)
Done.
options.go, line 329 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 110 characters (from
lll)
Done.
options.go, line 340 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 116 characters (from
lll)
Done.
options.go, line 351 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 118 characters (from
lll)
Done.
options.go, line 354 at r2 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
line is 109 characters (from
lll)
Done.
options.go, line 130 at r3 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
How is this statement true? You are changing the value of opt.Dir and then returning the same opt value. Isn't that the opposite of what this sentence says? Or does setting the function signature to
func (opt Options)instead offunc (opt *Options)means the copy is implicit?
Exactly, the receiver is a copy so the original value is never modified making it easier to understand what's going on.
manishrjain
left a comment
There was a problem hiding this comment.
Good change overall. The only concern is that each new field here in Options would now need a corresponding
WithX function. Maybe add a note in options to ensure that future changes need to keep that in mind.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @ashish-goswami, @campoy, @golangcibot, and @martinmr)
options.go, line 356 at r3 (raw file):
The original options value is never modified.
Doesn't seem like this needs to be mentioned repeatedly in every With func.
campoy
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @ashish-goswami, @golangcibot, @manishrjain, and @martinmr)
options.go, line 356 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
The original options value is never modified.
Doesn't seem like this needs to be mentioned repeatedly in every With func.
Done.
|
After verifying that the Windows errors are due to flaky tests and not a my PR, I'm merging. |
Broken currently here:
go: finding github.com/zippoxer/bow latest
go: github.com/dgraph-io/[email protected]+incompatible: go.mod has post-v2 module path "github.com/dgraph-io/badger/v2" at revision v2.0.0-rc.2
go: error loading module requirements
Which seems to be related to dgraph-io/badger#886
There's also a recent upstream API change in dgraph-io/badger#874
that requires additional changes.
Broken currently here:
go: finding github.com/zippoxer/bow latest
go: github.com/dgraph-io/[email protected]+incompatible: go.mod has post-v2 module path "github.com/dgraph-io/badger/v2" at revision v2.0.0-rc.2
go: error loading module requirements
Which seems to be related to dgraph-io/badger#886
There's also a recent upstream API change in dgraph-io/badger#874
that requires additional changes.
Since we're changing our API I'm thinking we should take the opportunity to break a couple more things
that will make our API nicer to use.
One of the complains I've previously seen is how the Open API is not idiomatic nor friendly.
Indeed it forces the user to first fetch the DefaultOptions, then modify them, to finally pass them.
I propose a simple change on which the default options are implicity and instead we simply pass Option
values which provide ways to set specific settings.
For instance the current way to open a database is:
With the new API, the code becomes:
If the user wants to open it in read-only mode:
It is much easier for beginners and still provides the same options as previously.
This change is