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

get code coverage up to 100% #53

Merged
merged 1 commit into from
Sep 9, 2016
Merged

get code coverage up to 100% #53

merged 1 commit into from
Sep 9, 2016

Conversation

nolanlawson
Copy link
Contributor

There were just a few changes necessary to get it to 100%. Also there was quite a bit of dead code.

@coveralls
Copy link

Coverage Status

Coverage increased (+6.9%) to 100.0% when pulling 45407b4 on code-coverage into 8cf8870 on master.

, createRBT = require('functional-red-black-tree')
, globalStore = {}

function toKey (key) {
return typeof key == 'string' ? '$' + key : JSON.stringify(key)
return '$' + key
Copy link
Member

Choose a reason for hiding this comment

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

this would remove support for objects as keys, right? like the key would then just always be $[object Object]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this is actually only used for database names. So you'd no longer be able to do new MemDOWN({'foo': 'bar'}) although I doubt anyone was doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I guess it may technically be a breaking change, although I've never heard of this feature being documented anywhere in the level ecosystem (levelup({foo: 'bar'})?) so I'd consider it dubious.

Copy link
Member

Choose a reason for hiding this comment

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

ah, ok understood. then let's rename this function, so it's more clear for the next reader? what about something like toLocation?

Copy link
Member

Choose a reason for hiding this comment

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

or just inline that code, it's not complex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can inline it, yeah.

@juliangruber
Copy link
Member

thank you for your work on this!

@nolanlawson
Copy link
Contributor Author

Added @juliangruber's suggestions and removed the setImmediate -> immediate change per discussion in #52

@coveralls
Copy link

Coverage Status

Coverage increased (+6.9%) to 100.0% when pulling cc7829c on code-coverage into 8cf8870 on master.

@nolanlawson
Copy link
Contributor Author

localtunnel is not having a good day today, which is why the tests keep failing. will wait for a green tho

@nolanlawson
Copy link
Contributor Author

I have restarted this like 10 times and localtunnel still keeps failing. 😕

@nolanlawson
Copy link
Contributor Author

wooooooot

@nolanlawson nolanlawson merged commit a794387 into master Sep 9, 2016
@juliangruber juliangruber deleted the code-coverage branch September 9, 2016 08:35
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