Skip to content

Support Autobee, second attempt#9

Open
Jampard wants to merge 6 commits intoRangerMauve:defaultfrom
Jampard:default
Open

Support Autobee, second attempt#9
Jampard wants to merge 6 commits intoRangerMauve:defaultfrom
Jampard:default

Conversation

@Jampard
Copy link
Copy Markdown

@Jampard Jampard commented Sep 2, 2022

Nice. Thank you for looking into this.

Is the purpose of having this new "base" parameter everywhere to overcome issues with autobee.sub not working correctly? Would it make sense to instead require autobee to have that method implemented correctly so that we don't need to add a special case for it?

It would be nice if autobee exposed an API similar to Hyperbee so that it could be passed into the hyperbeedeebee constructor without extra effort.

Also, mind removing the yarn.lock and adding it to the gitignore? Generally lockfiles are needed for applications rather than reusable libraries.

Hello Mauve,
Thanks a lot for the comment. I didn't thought it was possible to reach the same API, because of the sub function.
I gave it a go and it worked!
All the changes are now in the autodeebee.js file (ex SimpleAutobee)
Still got more tests to add specifically to Autobase. I tried it in my app and it works so it sounds good.

Cheers,
Jamps

Copy link
Copy Markdown
Owner

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

This is looking great so far, TY!

Just left some comments on how we can reduce the surface changed further and make the testing for both paths more uniform.

const Hyperbee = require('hyperbee')
const b4a = require('b4a')

module.exports = class Autodeebee {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Do you think you could publish this as a separate module which we could link to in the README for folks to test and include in the devDependencies for the tests?

index.js Outdated
return ensureComparable(docValue) <= ensureComparable(queryValue)
}

function compareNe (docValue, queryValue) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could this feature be split into a separate PR?

@@ -0,0 +1,1074 @@
const test = require('tape')
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you refactor test.js so that we can have all the tests take a getBee parameter which will be run with both the regular hyperbee and the autobee?

This way we don't need to duplicate all the test code and can make sure that new tests cover both sides.

e.g.

makeTests(getBee, 'Hyperbee')
makeTests(getAutoBee, 'Autobee')

function makeTests(getBee, type) {
  test(`${type} Create a document in a collection`, async (t) => {
  const db = new DB(getBee())
.... etc
}

@ducksandgoats
Copy link
Copy Markdown

i can't wait till this is merged. i wanna get started and work with a multiwriter hyperbeedeebee asap. hope this pr is not forgotten about or abandoned.

@Jampard
Copy link
Copy Markdown
Author

Jampard commented Sep 18, 2022

Hello,
Sorry had a lot on my plate, i am getting to it now :)
Thanks for the review @RangerMauve this is very appreciated.

@ducksandgoats
Copy link
Copy Markdown

Hello, Sorry had a lot on my plate, i am getting to it now :) Thanks for the review @RangerMauve this is very appreciated.

glad this is still being worked on

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.

3 participants