Skip to content
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

EJSON Improvements #1745

Closed
wants to merge 4 commits into from
Closed

EJSON Improvements #1745

wants to merge 4 commits into from

Conversation

@marcandre
Copy link
Contributor

@marcandre marcandre commented Jan 9, 2014

Here are the most straightforward changes of #1734:

  • test for custom types
  • fixing equals asymmetry
  • provide default clone
  • provide default equals
@marcandre marcandre mentioned this pull request Jan 9, 2014
@@ -0,0 +1,62 @@
function Address (city, state) {

This comment has been minimized.

@glasser

glasser Jan 15, 2014
Member

Can you call the file something like custom_models_for_tests.js? Useful to have test in the name of test files.

@@ -300,6 +305,10 @@ EJSON.equals = function (a, b, options) {
}
return true;
}
switch (EJSON._isCustomType(a) + EJSON._isCustomType(b)) {

This comment has been minimized.

@glasser

glasser Jan 15, 2014
Member

Comment that this is essentially a fallback for custom types that don't implement their own equals

@@ -300,6 +305,10 @@ EJSON.equals = function (a, b, options) {
}
return true;
}
switch (EJSON._isCustomType(a) + EJSON._isCustomType(b)) {
case 1: return false;
case 2: return EJSON.equals(EJSON.toJSONValue(a), EJSON.toJSONValue(b));

This comment has been minimized.

@glasser

glasser Jan 15, 2014
Member

I misread this as a.toJSONValue() at first, which would have been wrong. Can you put in a test that makes it clear that two different custom types whose x.toJSONValue() return identical structures are not equal?


var d = new Date;
var obj = new EJSONTest.Person("John Doe", d, a);
testCustomObject( obj );

This comment has been minimized.

@glasser

glasser Jan 15, 2014
Member

style nit: we don't put spaces around function arguments

@glasser
Copy link
Member

@glasser glasser commented Jan 15, 2014

Thanks!

This requires updating the docs. I'd move the typeName and toJSON value blocks above clone and equals (under addType) and make it clear that they are optional.

Looks good, I'll merge if you make the requested fixes.

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Jan 18, 2014

Great! All good points, now fixed.
For the documentation, I had made the changes already, let me know if what I did could be improved!

@glasser
Copy link
Member

@glasser glasser commented Jan 29, 2014

By docs, I mean the actual user-facing (docs.meteor.com) documentation in the docs directory (it's just a meteor app). It's a bit of a mess but you want docs/client/api.html and docs/client/api.js.

@marcandre
Copy link
Contributor Author

@marcandre marcandre commented Feb 11, 2014

Gotcha. Doc app updated.

glasser added a commit that referenced this pull request Feb 12, 2014
Text before an api_box does not look good as an introduction to that
api_box due to spacing.
glasser added a commit that referenced this pull request Feb 12, 2014
@glasser
Copy link
Member

@glasser glasser commented Feb 12, 2014

Thanks, merged!

@glasser glasser closed this Feb 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants