Improvements to new Julia API#67
Merged
Merged
Conversation
The catch branch called the undefined LMDB.is_notfound, so deleting a missing key would have crashed with UndefVarError. The test suite only ever deleted existing keys, masking the bug. It's also dead code: LMDB.delete!(txn, dbi, key) returns Bool and never throws on missing. Drop the try/catch entirely, matching the no-op shape of Base.delete!(::Dict).
HasLength() is already the default IteratorSize for any iterator that doesn't override it (Base/generator.jl:80), so the explicit override on LMDBDict adds nothing.
Clang.Generators emits `export $jll_pkg_name` whenever `jll_pkg_name` is set, which leaks the C-binding dependency into LMDB's user-facing API. Strip the export from the generated source and add a post-processing step so it stays gone across regenerations.
`show(io::IO, err)` controls how the struct is printed in any IO context (arrays, repr, logs), while `showerror` is what runs when an exception is actually thrown. Overriding `show` to print "Code[..]: .." made the struct render unusably and bypassed the canonical error path. Reroute the formatted message through `Base.showerror` and let `show` fall back to the default struct printing.
The previous single-method `show` always emitted multi-line output, so printing an `Environment` inside another container (an array, a tuple, or `repr`) produced a paragraph instead of a one-liner. Follow the convention used by `Dict` and other Base containers: the two-argument `show` is the compact reparseable form, and the three-argument `MIME"text/plain"` overload is the multi-line REPL display.
These all returned a meaningless Cint(0) — a holdover from the pre- @checked era when bindings handed back raw status codes. Replace them with the canonical forms: `close` and `sync` return `nothing`, `set!`/`unset!` return `env` so calls can chain.
Spell out that empty(::LMDBDict, K, V) returns an in-memory Dict, and test the resulting copy/merge/filter behavior. The Base default already delivers this (dict.jl:119), but writing the override makes the type contract explicit — LMDBDict can't construct itself without a path, so the fallback type is locked in by design rather than by accident.
Use Julia's idiomatic constructor + do-block form for every wrapper handle: Transaction(env; ...) / Transaction(f, env; ...) DBI(txn, name; ...) / DBI(f, txn, name; ...) Cursor(txn, dbi) / Cursor(f, txn, dbi) The Environment(path; ...) constructor already followed this shape; the do-block sibling is added too. The old `start`, `create`, `environment`, `open(txn)`, `open(env, path)`, and `open(txn, dbi)` factory functions go away — they were redundant with the constructors and used several different idioms for what was conceptually one operation (allocate handle + configure + register lifetime). Internal callers in `dictionary.jl`, the test suite, and the user docs are updated to the new form.
`isflagset` and `walk` were exported but are rarely needed outside the LMDB namespace: `isflagset` is a generic helper, and `walk` is mostly subsumed by AbstractDict iteration over LMDBDict. Move them to @public so callers reach them as `LMDB.isflagset` / `LMDB.walk`, freeing up the short names in user code.
cursor.jl's `setup_key!` ladder was a copy of the dispatch that
`common.jl` already runs through `MDBArg`. Replace it with a single
`fill_mdbval!(dst::Ref{MDB_val}, k)` helper that reuses the canonical
`cconvert(Ptr{MDB_val}, k)` carrier — one path for building MDB_vals,
one place to add support for new key types.
Replace `tryget(txn, dbi, key, T)` with `get(txn, dbi, key, T, nothing)`
— the same Union{T,Nothing} shape, expressed through the canonical
Base.get(d, k, default) signature so there is one name for the same
operation. Callers wanting `nothing` on miss pass `nothing`; callers
wanting another sentinel pass that instead.
Internal callers in `dictionary.jl`, replace! / pop!, the test suite,
and the docs follow.
The previous commit made empty/copy silently downgrade to an in-memory Dict. That hides the structural difference: an LMDBDict needs an on-disk directory, so there is no path-less zero-argument form. Throw ArgumentError instead, with a message pointing at `Dict(d)` for the in-memory snapshot or `copy(d.env, path)` for an on-disk clone. filter(f, d) goes through empty(d) and therefore also throws. merge builds a Dict via the AbstractDict iteration constructor, so it continues to work and returns the merged Dict (same shape as the user would get from Dict(d) by hand).
Only `LMDBDict` and `LMDBError` are still exported. Everything else — `Environment`, `Transaction`, `DBI`, `Cursor`, the env operations (`set!`/`unset!`/`sync`/`info`/`path`/`reader_check`/`reader_list`), the cursor primitives (`seek!`, `next!`, `prev!`, `walk`, dup variants), and the cursor accessors (`transaction`/`database`/ `key`/`value`/`item`) — moves to `@public`. The bare names are too generic to safely take over downstream namespaces under `using LMDB`. Reach for them as `LMDB.Transaction`, `LMDB.seek!`, etc. The test suite pulls a fixed list of the common names back into scope via `using LMDB: ...` at the top of `runtests.jl` so the tests stay readable while the public surface stays narrow.
`Database` reads better than the C-flavoured `DBI`. The struct, all its method signatures, and every prose mention switch over. The underlying `MDB_dbi` Cuint type (the C ABI) is unchanged; only the Julia wrapper renames. While here, switch the tests from bare names to the `LMDB.X` qualification that user code actually sees after the recent export demotion. `LMDBDict` / `LMDBError` stay unprefixed (still exported); `LMDB.Environment`, `LMDB.Transaction`, `LMDB.Database`, `LMDB.Cursor`, and the env operations (`LMDB.set!`, `LMDB.unset!`, `LMDB.sync`, `LMDB.info`, `LMDB.reader_check`, `LMDB.reader_list`) gain the prefix. The earlier `using LMDB: ...` hack in runtests.jl is gone.
Replace the slightly stiff "Get an item from a database, decoded as T" phrasing on `get` with "Read the value at `key`, decoded as T", and swap "whether or not f throws" on `Environment(f, path)` for the plainer "even if f throws".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
- Coverage 83.50% 82.40% -1.10%
==========================================
Files 9 9
Lines 776 756 -20
==========================================
- Hits 648 623 -25
- Misses 128 133 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some renames, dead code removal, and better alignment with Base APIs.