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

Null handling #222

Closed
wants to merge 4 commits into from
Closed

Null handling #222

wants to merge 4 commits into from

Conversation

robocoder
Copy link
Contributor

I'm resubmitting this patch, with commits separated for easier review.

(See individual commit for more info.)

…onstructor.

Add tests to BaseSerializationTest:
* testNull when expecting types
* testNumerics when expecting numeric types
* testDateTime
* testDeserializingNull when using InitializedObjectConstructor

Add content for blog_post_unauthored and date_time:
* JsonSerializationTest
* xml/blog_post_unauthored.xml
* xml/date_time.xml
* yml/blog_post_unauthored.yml
* yml/date_time.yml

So, from the phpunit test results, we see:
* fails when serializing a simple DateTime
* error when deserialing a null when expecting a DateTime
* error: no support for the "float" alias for "double"
* error when deserializing: a null value will not overwrite a non-null property in the initialized object
* fix serialization/deserialization of DateTime
* there's no change to parseDateTime() other than removing DOS carriage returns
return $visitor->visitNull(null, $type);
}

return $visitor->visitString($date->format($this->getFormat($type)), $type);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why writing exactly the same method 3 times ? you should keep the code DRY

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and will refactor after @schmittjoh reviews it.

@schmittjoh schmittjoh closed this in f388a84 Dec 1, 2012
@schmittjoh
Copy link
Owner

I've merged some commits of this PR (mostly tests), and refactored some other parts to use the changes I've made to master already.

Everything passed now, let me know if there are any problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants