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

Implement .has() #75

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dusansimic
Copy link
Contributor

dusansimic commented Jan 22, 2019

Implementing .has() method from Map instances. Also adding .isUndefined() method since there's been a request for it in an issue referenced below.
The idea is to expose more raw data from Keyv via creating a method .has() that just tells us if the specified key exists and .isUndefined() method that tells us if that key has a value.

Closes: #50

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 22, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling b4eec55 on dusansimic:implement-has into 571be4f on lukechilds:master.

@lukechilds
Copy link
Owner

lukechilds left a comment

While you could pass this through to the storage adapter with store.has() you'd then need to implement a has() method on all storage adapters for this to work and it would break all existing third-party storage adapters.

I'd suggest a better solution would be to allow passing an option to get() to return the raw value and checking if the raw value is set.

That should work without requiring any changes to the official storage adapters and without breaking third-party adapters.


return Promise.resolve()
.then(() => {
return store.has(key);

This comment has been minimized.

@lukechilds

lukechilds Jan 27, 2019

Owner

You would need to implement the .has() method on all official storage adapters before this would work.

});
}

isUndefined(key) {

This comment has been minimized.

@lukechilds

lukechilds Jan 27, 2019

Owner

I'm not sure an .isUndefined() method is necessary, it can be done outside of Keyv once .has() is implemented.

@dusansimic

This comment has been minimized.

Copy link
Contributor Author

dusansimic commented Jan 27, 2019

I'd suggest a better solution would be to allow passing an option to get() to return the raw value and checking if the raw value is set.

This is already implemented in #42. It just needs to be pulled.

@lukechilds

This comment has been minimized.

Copy link
Owner

lukechilds commented Feb 4, 2019

Woops sorry, just seen #42 is over a year old!

Apologies for the delay, it's been a very crazy year and have been struggling to keep up with OSS, will take a look at that PR now.

@lukechilds

This comment has been minimized.

Copy link
Owner

lukechilds commented Feb 4, 2019

@dusansimic I've reviewed #42 and made a few tweaks and merged.

If you could update this PR to make use of the raw option that would be brilliant.

Sorry again for the delay on that!

dusansimic added some commits Jan 21, 2019

Implemented has method
Implemented has method so set will only accept values that are NOT
undefined and has will check if get method result is undefined. It will
be only if the key doesn't exist.
Separate .has and .isUndefined to make it more raw
To make Keyv return more raw data which will help with creating modules,
.has method is separated from .isUndefined method so .has just tells us
if the key exists and .isUndefined if it has a value.

@dusansimic dusansimic force-pushed the dusansimic:implement-has branch from 453c8e4 to 675a60d Feb 4, 2019

Remove isUndefined() and update has()
Remove isUndefined() function from index and test as requested. Also
update has() function so it gets raw value of a key and checks if it
exsits that way.
@lukechilds
Copy link
Owner

lukechilds left a comment

You should just do a raw .get() inside .has() and check if the raw value is set or not.


return Promise.resolve()
.then(() => {
return Boolean(store.get(key, { raw: true }));

This comment has been minimized.

@lukechilds

lukechilds Feb 5, 2019

Owner

This won't work, store references the underlying storage adapter which likely won't support the raw option.

This comment has been minimized.

@dusansimic

dusansimic Feb 13, 2019

Author Contributor

So it should be called via this?

@@ -86,6 +86,16 @@ class Keyv extends EventEmitter {
.then(() => true);
}

has(key) {
key = this._getKeyPrefix(key);

This comment has been minimized.

@lukechilds

lukechilds Feb 5, 2019

Owner

You don't need to re-implement .get() inside .has(), you can just call .get() inside it with { raw: true }.

This comment has been minimized.

@dusansimic

dusansimic Feb 13, 2019

Author Contributor

Done

Add requested changes
Get boolean weather key exisits by running .get() with raw option inside
.has(). Also calling .get() method via this.

@dusansimic dusansimic changed the title Implement .has() and .isUndefined() Implement .has() Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment