Implement .has() #75

dusansimic · 2019-01-22T10:24:00Z

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 · 2019-01-22T10:27:11Z

Coverage decreased (-1.3%) to 98.734% when pulling ed7df7c on dusansimic:implement-has into 571be4f on lukechilds:master.

lukechilds

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.

lukechilds · 2019-01-27T09:32:26Z

src/index.js

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


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

lukechilds · 2019-01-27T09:32:31Z

src/index.js

+			});
+	}
+
+	isUndefined(key) {


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

dusansimic · 2019-01-27T15:34:29Z

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 · 2019-02-04T04:10:08Z

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 · 2019-02-04T05:31:11Z

@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!

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.

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.

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

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

lukechilds · 2019-02-05T04:21:42Z

src/index.js

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


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

So it should be called via this?

lukechilds · 2019-02-05T04:23:43Z

src/index.js

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

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


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

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

lukechilds · 2019-03-02T00:11:22Z

src/index.js

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

+	async has(key) {
+		return Boolean(await this.get(key, { raw: true }));


Here I would probably check like this

(typeof await this.get(key, { raw: true }) === 'object')

Although not sure that's necessarily better. Any specific reason you chose to wrap with Boolean()?

// CC @sindresorhus any input would be appreciated here 🙏

Well it's just something I do so I would know I want to know if that value exists or not. If I use typeof it look to me like I'm checking for object specifically but what I actually want anything except null/undefined. I'd also like Sindres input on this.

If I use typeof it look to me like I'm checking for object specifically but what I actually want anything except null/undefined.

That's exactly why I don't think it should be written like this. I think you should be checking for an object explicitly.

Using {raw: true} will return the raw keyv object if it's set or undefined if it's not. So you literally want to say "Does this function return an object." or "Does this function not return undefined"

So

typeof (await this.get(key, { raw: true })) === 'object'

or

(await this.get(key, { raw: true })) !== undefined

are semantically correct.

What you're currently doing:

Boolean(await this.get(key, { raw: true }));

is just saying "Is this value truthy".

Which happens to work in this scenario, an object will be truthy and undefined will be falsey. However it's not checking the exact value we know it should be. It's less explicit and it's possible if we change the way data is structured internally it could break.

Does that make sense?

lukechilds · 2019-03-02T00:20:51Z

test/keyv.js

@@ -85,6 +85,14 @@ test.serial('.get(key, {raw: true}) returns the raw object instead of the value'
 	t.is(rawObject.value, 'bar');
 });

+test.serial('.has(key) where key is the key we are looking for', async t => {


These tests should actually be in keyv-test-suite so they are run against all storage adapters and catch any anomalies.

Don't worry about that for now, you can add the tests here and I'll move them over. The development process is a bit painful because all the modules depend on each other.

Would you be able to make the tests a bit more extensive though, it should test all possible different types of behaviour with all types. Ideally a single test for each check.

It may seem over the top and it's fine with the current implementation because you know it'll always return an object or undefined. But that's testing the current implementation, not the intended functionality.

If in the future we made a change to how entries are stored internally it could mean that a value of undefined would then cause .has() to return false when it should return true.

You should extensively test all expected behaviours with all types.

You can use these test files for inspiration:
https://github.com/lukechilds/keyv-test-suite/blob/master/src/api.js
https://github.com/lukechilds/keyv-test-suite/blob/master/src/values.js

Check if value is `undefined` instead of using `Boolean()` function for converting it to boolean.

dusansimic · 2019-11-17T01:07:10Z

I'm sorry for not finishing this up earlier, I had a lot of work around university. I've added all the requested changes and also created a PR on keyv-test-suite for the has() tests.

Kikobeats · 2021-07-11T11:27:16Z

Implemented at https://keyv.js.org 👍

lukechilds suggested changes Jan 27, 2019

View reviewed changes

dusansimic added 2 commits February 4, 2019 11:45

Implemented has method

bfa477a

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

675a60d

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 force-pushed the implement-has branch from 453c8e4 to 675a60d Compare February 4, 2019 10:58

Remove isUndefined() and update has()

45d1635

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 suggested changes Feb 5, 2019

View reviewed changes

Add requested changes

b4eec55

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

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

lukechilds reviewed Mar 2, 2019

View reviewed changes

lukechilds suggested changes Mar 2, 2019

View reviewed changes

dusansimic added 2 commits November 14, 2019 12:14

Check for undefined instead of parsing to boolean

7565970

Check if value is `undefined` instead of using `Boolean()` function for converting it to boolean.

Move tests for has() to keyv-test-suite

ed7df7c

Kikobeats closed this Jul 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement .has() #75

Implement .has() #75

dusansimic commented Jan 22, 2019

coveralls commented Jan 22, 2019 •

edited

lukechilds left a comment •

edited

lukechilds Jan 27, 2019 •

edited

lukechilds Jan 27, 2019

dusansimic commented Jan 27, 2019 •

edited

lukechilds commented Feb 4, 2019

lukechilds commented Feb 4, 2019

lukechilds left a comment

lukechilds Feb 5, 2019

dusansimic Feb 13, 2019

lukechilds Feb 5, 2019

dusansimic Feb 13, 2019

lukechilds Mar 2, 2019

dusansimic Mar 2, 2019

lukechilds Mar 2, 2019

lukechilds Mar 2, 2019 •

edited

dusansimic commented Nov 17, 2019

Kikobeats commented Jul 11, 2021

Implement .has() #75

Implement .has() #75

Conversation

dusansimic commented Jan 22, 2019

coveralls commented Jan 22, 2019 • edited

lukechilds left a comment • edited

Choose a reason for hiding this comment

lukechilds Jan 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusansimic commented Jan 27, 2019 • edited

lukechilds commented Feb 4, 2019

lukechilds commented Feb 4, 2019

lukechilds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukechilds Mar 2, 2019 • edited

Choose a reason for hiding this comment

dusansimic commented Nov 17, 2019

Kikobeats commented Jul 11, 2021

coveralls commented Jan 22, 2019 •

edited

lukechilds left a comment •

edited

lukechilds Jan 27, 2019 •

edited

dusansimic commented Jan 27, 2019 •

edited

lukechilds Mar 2, 2019 •

edited