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

Add clear() #669

Merged
merged 4 commits into from
Sep 8, 2019
Merged

Add clear() #669

merged 4 commits into from
Sep 8, 2019

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Aug 18, 2019

@vweevers vweevers added enhancement New feature or request semver-minor New features that are backward compatible labels Aug 18, 2019
@vweevers vweevers added the blocked Unable to proceed with this issue or pull request label Aug 18, 2019
@@ -446,6 +464,7 @@ const main = async () => {
| `put` | Key has been updated | `key, value` (any) |
| `del` | Key has been deleted | `key` (any) |
| `batch` | Batch has executed | `operations` (array) |
| `clear` | Entries were deleted | `options` (object) |
Copy link
Member Author

Choose a reason for hiding this comment

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

Not 100% sure about the argument (options). I figured a listener might want to inspect the range options, to find out what was deleted. But that information is not exact (in contrast with the put, del and batch events that tell you exactly which keys were modified).

@vweevers vweevers removed the blocked Unable to proceed with this issue or pull request label Sep 6, 2019
@vweevers
Copy link
Member Author

vweevers commented Sep 8, 2019

@ralphtheninja Before we release this I want to think about adding a way to feature-detect it.

A module like noffle/kappa-view-level (used in cabal-core) may want to use db.clear() for its clearIndex(). If cabal-core were to switch to level-mem (instead of the old memdb that it currently uses) then the db it passes into kappa-view-level would indeed have a clear() method. Problem is, both these modules support passing in a custom db.

That db may be levelup with clear(), but its underlying db may not have clear(). So a check like typeof db.clear === 'function' would not suffice. Even worse, the db may be subleveldown with the default clear() of abstract-leveldown and delete keys outside of the sublevel range :(

@ralphtheninja
Copy link
Member

Maybe we could start by adding a clear() to subleveldown to prevent from falling back to default behavior? But I guess the damage is already done there if it already falls back to using default clear().

@vweevers
Copy link
Member Author

vweevers commented Sep 8, 2019

Yeah, we could make a patch release on subleveldown to either pin abstract-leveldown or add a noop _clear(). No one is using clear() yet and it isn't exposed on the outer levelup instance yet so we have time.

@vweevers
Copy link
Member Author

vweevers commented Sep 8, 2019

It's time for manifests (again), I think. Level/community#42 (comment)

@ralphtheninja
Copy link
Member

It's time for manifests (again), I think. Level/community#42 (comment)

I was thinking manifest just now :)

@vweevers
Copy link
Member Author

vweevers commented Sep 8, 2019

I propose the following timeline:

  1. Patch subleveldown
  2. Release levelup minor with clear() (floats into level and friends)
  3. Start working on manifests, as the "next big thing". In parallel, add clear() to subleveldown

Edit for the record: it turned out clear() worked out of the box: Level/subleveldown#73 (comment).

@vweevers vweevers merged commit 45e73a9 into master Sep 8, 2019
@vweevers vweevers deleted the add-clear branch September 8, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request semver-minor New features that are backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clear() method
2 participants