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

feat: Pickle DB Backend Support #37

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

nx2k3
Copy link

@nx2k3 nx2k3 commented Apr 18, 2023

This adds support for Pickle DB alternative backend for non-wasm based environments.
#32

@nx2k3
Copy link
Author

nx2k3 commented Apr 18, 2023

Is error handling done right

Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Thanks!

Apart from the unused variant, I think the error handling is right, in the sense that it follows the current convention of the other backends. We might want to revisit that though, but not in this PR.

There are some whitespace formatting issues, most of them should go away by running cargo fmt, but the toml ones you'll have to fix manually.

Other than that, it looks great to me :)

Cargo.toml Outdated Show resolved Hide resolved
src/pickledb_store.rs Outdated Show resolved Hide resolved
src/pickledb_store.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@johanhelsing johanhelsing added enhancement New feature or request native labels Apr 18, 2023
src/pickledb_store.rs Outdated Show resolved Hide resolved
@johanhelsing
Copy link
Owner

Are the tests passing locally for you?

@nx2k3
Copy link
Author

nx2k3 commented Apr 20, 2023

Are the tests passing locally for you?

yes they are passing

@johanhelsing
Copy link
Owner

Hmm.. okay. We need to figure out why they are failing in ci, then. Which os are you on?

@nx2k3
Copy link
Author

nx2k3 commented Apr 20, 2023

Hmm.. okay. We need to figure out why they are failing in ci, then. Which os are you on?

arch linux wsl
image

@johanhelsing
Copy link
Owner

Okay i tried on windows, passing there as well.

Maybe it's some parent directory that needs to be created or something? What happens if you delete the app folder? Do the tests fail then?

Guess i could try this myself, but on the go now

@nx2k3
Copy link
Author

nx2k3 commented Apr 21, 2023

hey i think dump function is causing error

@johanhelsing johanhelsing added the help wanted Extra attention is needed label May 6, 2023
Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

I think pickledb is a really good fit for a new default backend on native... but we need to fix that test. If anyone has the time to look into it, it would be much appreciated!

@Dimchikkk
Copy link
Contributor

@johanhelsing I looked into pickledb source code, I might be wrong but it seems it loads the whole DB into memory which is a huge blocker for me to use.

@johanhelsing
Copy link
Owner

Ah, bummer. In either case, I don't plan on getting rid of the other backends.

Other suggestions for maintained rust-only native backends are welcome.

@nx2k3
Copy link
Author

nx2k3 commented Oct 8, 2023

hey I think its better to close this PR as redb support was added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants