-
Notifications
You must be signed in to change notification settings - Fork 79
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
fix(formatting): Date/RegExp values output by formatComplexDataStructure #250
fix(formatting): Date/RegExp values output by formatComplexDataStructure #250
Conversation
There were the following issues with this Pull Request
|
9dbbec8
to
a9e01e3
Compare
sortobject
for own implementationsortobject
for own implementation
sortobject
for own implementationThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 👍 We will definitely merge this!
Do not forget to rebase the PR too 😃
@@ -86,4 +86,20 @@ describe('formatComplexDataStructure', () => { | |||
it('should format an empty array', () => { | |||
expect(formatComplexDataStructure([], false, 0, options)).toEqual('[]'); | |||
}); | |||
|
|||
it('should format an object that contains a date', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess theses tests are more related to sortObject
behavior than the formatComplexDataStructure
one. We should move then in the sortObject.spec.js
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sortObject
has its own test for that. I think left it here since not only sortObject
is used to process objects and other logic can break things too.
src/formatter/sortObject.spec.js
Outdated
|
||
import sortObject from './sortObject'; | ||
|
||
describe('formatComplexDataStructure', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oups! Should be describe('sortObject', () => {
c: date, | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test that validate that sortObject
could be directly call with array sortObject([{...}, {...}])
;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
a9e01e3
to
c9060a7
Compare
Done :) |
sortobject
breaks output of Date and RegExp values, since makes an object copy of those values instead returning as is. Therefore,formatComplexDataStructure
for following objectoutputs
We can fix
sortobject
by sending a PR. But its implementation is too verbose for such simple task. So I decided to add an own implementation ofsortObject
instead of patchingsortobject
. This also helps keep things simpler and allows adjust behaviour if needed.