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

Abstract tests single function #495

Merged
merged 4 commits into from
Jul 13, 2018
Merged

Conversation

ralphtheninja
Copy link
Member

No description provided.

const testCommon = require('./common')
const abstract = require('abstract-leveldown/test/close-test')

test('setUp', testCommon.setUp)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we're skipping setUp and tearDown now.

const testCommon = require('./common')
const abstract = require('abstract-leveldown/test/close-test')

test('setUp', testCommon.setUp)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

And an .all() method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make an issue and hack it up.

}

module.exports = testCommon
module.exports = require('abstract-leveldown/test/common')({
Copy link
Member

Choose a reason for hiding this comment

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

Ah this comes in handy

@ralphtheninja
Copy link
Member Author

I'll update this PR once Level/abstract-leveldown#278 has been merged

@ralphtheninja
Copy link
Member Author

I get the following error now:

screenshot from 2018-07-13 18-09-28

@vweevers
Copy link
Member

@ralphtheninja we have to update chained batch.

@ralphtheninja
Copy link
Member Author

@ralphtheninja we have to update chained batch.

I'll look into it.

@vweevers
Copy link
Member

The C++ takes the first argument as a function, should be the second:

new Nan::Callback(v8::Local<v8::Function>::Cast(info[0]));

@vweevers
Copy link
Member

Hmm I now see that levelup doesn't take options in Batch.write either: https://github.com/Level/levelup/blob/c3845eea4547a829b05548c092b84d5cc30b8e46/lib/batch.js#L50

So perhaps we should just remove the options argument from abstract-leveldown?

@ralphtheninja
Copy link
Member Author

ralphtheninja commented Jul 13, 2018

So perhaps we should just remove the options argument from abstract-leveldown?

I think it's fine to pass on options to _write(). We can either omit passing it to the C++ layer (just ignoring options) in leveldown or change on the C++ side.

It's more consistent passing options further.

@vweevers
Copy link
Member

It'd be dead code though (effectively), unless we also add an options argument to levelup.

@ralphtheninja
Copy link
Member Author

It'd be dead code though (effectively), unless we also add an options argument to levelup.

So lets do that :)

@ralphtheninja
Copy link
Member Author

It'd be dead code though (effectively), unless we also add an options argument to levelup.

It could default to {} in levelup so we can release it as a patch.

@vweevers
Copy link
Member

Alright, SGTM

@ralphtheninja
Copy link
Member Author

@vweevers Make an issue?

@vweevers
Copy link
Member

=> Level/levelup#603

@ralphtheninja ralphtheninja merged commit 86d97f5 into next Jul 13, 2018
@ralphtheninja ralphtheninja deleted the abstract-tests-single-function branch July 13, 2018 17:59
vweevers pushed a commit that referenced this pull request Sep 22, 2018
* Target abstract-leveldown#ffba12f

* Use single test function from abstract-leveldown

* Target abstract-leveldown#d7411bb

* Options is actually now passed as options from abstract-leveldown and not a function
vweevers pushed a commit that referenced this pull request Sep 22, 2018
* Target abstract-leveldown#ffba12f

* Use single test function from abstract-leveldown

* Target abstract-leveldown#d7411bb

* Options is actually now passed as options from abstract-leveldown and not a function
@rvagg rvagg mentioned this pull request Dec 18, 2018
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.

2 participants