feat(assertObjectMatchSpec): Compare object keys when doing a full match #56
Conversation
tests/core/assertions.test.js
Outdated
| }) | ||
|
|
||
| test('should allow to count nested objects properties', () => { | ||
| test('should allow to build an array of object propertieswith nested objects properties', () => { |
There was a problem hiding this comment.
| test('should allow to build an array of object propertieswith nested objects properties', () => { | |
| test('should allow to build an array of object properties with nested objects properties', () => { |
src/core/assertions.js
Outdated
| */ | ||
| exports.countNestedProperties = (object) => { | ||
| let propertiesCount = 0 | ||
| exports.buildObjectKeysArray = (object, dottedObjectKeys = [], currentPath = '') => { |
There was a problem hiding this comment.
nit: what about something like objectKeysDeep? Feels more aligned with the native API and other tools like lodash.
There was a problem hiding this comment.
Yup, better name for sure
src/core/assertions.js
Outdated
| */ | ||
| exports.countNestedProperties = (object) => { | ||
| let propertiesCount = 0 | ||
| exports.buildObjectKeysArray = (object, dottedObjectKeys = [], currentPath = '') => { |
There was a problem hiding this comment.
| exports.buildObjectKeysArray = (object, dottedObjectKeys = [], currentPath = '') => { | |
| exports.buildObjectKeysArray = (object, keysAccumulator = [], parentPath = '') => { |
src/core/assertions.js
Outdated
| let propertiesCount = 0 | ||
| exports.buildObjectKeysArray = (object, dottedObjectKeys = [], currentPath = '') => { | ||
| Object.keys(object).forEach((key) => { | ||
| if (!_.isEmpty(object[key]) && typeof object[key] === 'object') { |
There was a problem hiding this comment.
| if (!_.isEmpty(object[key]) && typeof object[key] === 'object') { | |
| if (!_.isEmpty(object[key]) && _.isPlainObject(object[key])) { |
There was a problem hiding this comment.
A better test for sure, but as we also take into account nested arrays, i also added a condition for it, which impact your next comment :)
src/core/assertions.js
Outdated
| exports.countNestedProperties = (object) => { | ||
| let propertiesCount = 0 | ||
| exports.buildObjectKeysArray = (object, dottedObjectKeys = [], currentPath = '') => { | ||
| Object.keys(object).forEach((key) => { |
There was a problem hiding this comment.
I think we could add a guard and throw with a meaningful message in case object is actually not an Object, what do you think?
There was a problem hiding this comment.
With new conditions, if an object (or an array) is passed, you will get a result, otherwise an empty array is returned, and the exact assertions between spec and object will fail. Maybe not the best way to do it, should need some improvement, but the recursive nature of objectKeysDeep, will force us to detect if it's the first iteration and then validate the given input. Maybe should we check object correctness in assertObjectMatchSpec ?
There was a problem hiding this comment.
Good point! Yea, we could do this in assertObjectMatchSpec.
src/core/assertions.js
Outdated
| * Make an array of keys from an object. | ||
| * Use a dot notation | ||
| * Only empty object keys are taken into account | ||
| * For subobjects only keys are taken into account |
There was a problem hiding this comment.
| * Make an array of keys from an object. | |
| * Use a dot notation | |
| * Only empty object keys are taken into account | |
| * For subobjects only keys are taken into account | |
| * Acts as `Object.keys()`, but runs recursively, | |
| * another difference is that when one of the key refers to | |
| * a non-empty object, it's gonna be ignored. | |
| * | |
| * Keys for nested objects are prefixed with their parent key. | |
| * | |
| * Also note that this is not fully interoperable with `lodash.get` | |
| * for example as keys themselves can contain dots or special characters. |
| beforeEach(() => {}) | ||
|
|
||
| test('should allow to count object properties', () => { | ||
| test('should allow to build an array of object properties', () => { |
There was a problem hiding this comment.
I'd suggest to add a test where we pass an invalid object, and also a test where properties contain dots or spaces.
There was a problem hiding this comment.
I've added some, may need rework depending on what we decide :)
leguellec
force-pushed
the
change-assertObjectMatchSpec
branch
from
0bb0628 to
67016ef
Compare
leguellec
force-pushed
the
change-assertObjectMatchSpec
branch
from
67016ef to
2af2b43
Compare
| } | ||
| }) | ||
|
|
||
| // We check we have exactly the same number of properties as expected |
There was a problem hiding this comment.
| // We check we have exactly the same number of properties as expected | |
| // We check we have exactly the same properties as expected |
There was a problem hiding this comment.
Or just remove this comment
pebie
force-pushed
the
master
branch
3 times, most recently
from
2a1f5cd to
7fa350d
Compare
resolves #44
As proposed by @plouc, the current way to fully match a json object to a given spec, was a bit too naïve. The introduced changes now handles the comparison based on object keys and spec keys. In consequences, a spec can now have multiple matcher on an object fields without impact on a full match.