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

Location was removed from abstract-leveldown #494

Merged
merged 2 commits into from
Jul 13, 2018

Conversation

ralphtheninja
Copy link
Member

  • handle location in LevelDOWN constructor
  • pass a factory function to abstract tests
  • use tempy and remove cleanup code

* handle location in LevelDOWN constructor
* pass a factory function to abstract tests
* use tempy and remove cleanup code
@ralphtheninja
Copy link
Member Author

@vweevers How do you see us using this branch? Will it be a long lasting branch? It will always target abstract-leveldown#master so I guess we can cherry-pick from this onto master branch once a new abstract-leveldown has been released?

@vweevers
Copy link
Member

It will always target abstract-leveldown#master so I guess we can cherry-pick from this onto master branch once a new abstract-leveldown has been released?

Or rebase to remove the first commit, then next can be merged into master. I don't know, we'll see, also depends on what other stuff happens on master in the mean time.

Main reason I suggested this approach is to have a way to canary test abstract-leveldown and keep discussion on unforeseen issues focused (by having small PRs). I wasn't thinking too much about what happens after.

@ralphtheninja
Copy link
Member Author

Main reason I suggested this approach is to have a way to canary test abstract-leveldown and keep discussion on unforeseen issues focused (by having small PRs). I wasn't thinking too much about what happens after.

It's a good idea. We should have this on memdown, level-js and rocksdb as well.

leveldown.js Outdated
AbstractLevelDOWN.call(this, location)
AbstractLevelDOWN.call(this)

this.location = location
Copy link
Member

Choose a reason for hiding this comment

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

'non-string location leveldown() throws'
)
t.end()
})
Copy link
Member Author

@ralphtheninja ralphtheninja Jul 13, 2018

Choose a reason for hiding this comment

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

This build will fail since abstract-leveldown#master has moved.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should target commit refs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Funny thing, the tests doesn't fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just goes from

screenshot from 2018-07-13 11-19-58

to

screenshot from 2018-07-13 11-19-21

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we should target commit refs?

I'll target a commit ref in next PR.

@ralphtheninja ralphtheninja merged commit 85ad5de into next Jul 13, 2018
@ralphtheninja ralphtheninja deleted the abstract-leveldown-remove-location branch July 13, 2018 14:58
vweevers pushed a commit that referenced this pull request Sep 22, 2018
* Location was removed from abstract-leveldown

* handle location in LevelDOWN constructor
* pass a factory function to abstract tests
* use tempy and remove cleanup code

* Add type check for location in constructor
vweevers pushed a commit that referenced this pull request Sep 22, 2018
* Location was removed from abstract-leveldown

* handle location in LevelDOWN constructor
* pass a factory function to abstract tests
* use tempy and remove cleanup code

* Add type check for location in constructor
@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