redis add jSet function
#716
Conversation
packages/redis/src/Adaptor.js
Outdated
| @@ -191,6 +191,31 @@ export function hset(key, value) { | |||
| }; | |||
| } | |||
|
|
|||
| /** | |||
| * Creates a JSON object at the key | |||
| * @example <caption>Set a field and value for the patient key</caption> | |||
There was a problem hiding this comment.
Are they still called fields and values in JSON?
I think this is just "setting an object to the value of key". And you only need one example, you don't need a multi-key example because that's not how JSON works
There was a problem hiding this comment.
Can be a JSON object or a JSON string, let me update the docs.
packages/redis/src/Adaptor.js
Outdated
| * @function | ||
| * @public | ||
| * @param {string} key - The key to modify. | ||
| * @param {object} value - The JSON value to set |
There was a problem hiding this comment.
Is it a json object, rather than a JSON value? Can you set an array, string, or boolean value?
There was a problem hiding this comment.
You can set a JSON string or a JSON object, A JSON can be deeply nested with values that are boolean or an array
packages/redis/test/Adaptor.test.js
Outdated
| @@ -411,3 +420,57 @@ describe('hGetAll', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('jSet', () => { | |||
| const jGetClient = { | |||
There was a problem hiding this comment.
Needs renaming to jSetClient
| const jGetClient = { | ||
| json: { | ||
| set: async (key, path, values) => { | ||
| expect(path).to.eql('$'); |
There was a problem hiding this comment.
Ah, this is important. Probably important enough for an explicit test, like jSet always sets at the document root
An explicit test would remind us and other developer that we are hard-coding the path to the root, which is an important detail. Test-as-documentation.
There was a problem hiding this comment.
I have added a test for should always sets results at the document root. Please share feedback if that's the right way of testing
packages/redis/test/Adaptor.test.js
Outdated
| setMockClient({ | ||
| json: { | ||
| set: (key, path, values) => { | ||
| expect(path).to.eql('$'); |
There was a problem hiding this comment.
I don't think these assertions are valuable or useful in this test. We've already proved that the right values get passed through
| expect(path).to.eql('$'); | ||
| expect(key).to.eql('hash-key'); | ||
| expect(values).to.eql({ name: 'mammoth' }); | ||
| throw new Error('Existing key has wrong Redis type'); |
There was a problem hiding this comment.
Is this exactly the same error as Redis throws?
I'm a bit torn on this test. Ok, it's good that it's documenting a certain behavior. But this isn't a behavior driven by our adaptor code
I suspect redis might throw a whole bunch of errors. What's special about this one?
There was a problem hiding this comment.
Yeah that is thrown by redis when you try to update a key that has a different redis type. For example if you already have an existing key that is a has-key and you want to update the values for that key, Redis will throw Existing key has wrong Redis type
| } | ||
| }); | ||
|
|
||
| it('should always sets results at the document root', async () => { |
There was a problem hiding this comment.
@mtuchi I've updated this test to what I meant.
Notice that it's only asserting one thing. That's the only thing this test cares about. Super focused.
It's technically redundant because we've already got an implied test for it, but for me it's worth calling out like this
There was a problem hiding this comment.
Thanks @josephjclark that makes lot of sense
Summary
Add
jSet(key: string, values: object)function.Ref #709
Notes
changeset is in
release/redisbranchReview Checklist