Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allowing null properties in resources #26

Merged
merged 5 commits into from May 1, 2012

Conversation

Projects
None yet
2 participants
@ghost

ghost commented Apr 26, 2012

Hi,

Thanks for writing halbuilder! I had a need to adapt it for a domain in which properties can have null values, and where those null values are significant (preferable to optional properties). The only ref I can find seems to agree that we should have the ability to use nulls in the app domain space (see http://groups.google.com/group/hal-discuss/browse_thread/thread/b5d55332ec7da702/3b6ace55adc30c19?lnk=gst&q=null#3b6ace55adc30c19 for the discussion), so I went ahead and added that capability to my branch of the project.

Rather than mess with ImmutableMap, I just created a separate member to hold null properties and added them to the appropriate readers and renderers.

Anyway, I wanted to offer up the changes in case you think they would be useful.

Mark Derricutt » halbuilder #16 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

Mark Derricutt » halbuilder #17 SUCCESS
This pull request looks good
(what's this?)

Implementing review suggestions:
 - Making property values Optional<Object> instances instead of maintaining a separate collection for them.
 - Simplifying rendering logic to account for the Optional objects.
 - Adding test case for ensuring that literal "null" values are treated as the string "null" instead of as null values.

Mark Derricutt » halbuilder #18 SUCCESS
This pull request looks good
(what's this?)

@ghost

ghost commented Apr 28, 2012

Thanks for the comments - I've pushed your suggested changes. Please take a look.

Implementing review suggestions:
 - Removing fromNullable from base resource's get() impl.
 - Updating InterfaceSatisfactionTest to contain a contract where the property value should be null for the contract to be satisfied.
@ghost

ghost commented Apr 29, 2012

Thanks for the comments, I've pushed changes. As you can probably tell, I'm new to Guava stuff, so this has been enlightening for me. :)

Is that addition to the unit test what you had in mind, or did I miss the boat there?

Mark Derricutt » halbuilder #19 SUCCESS
This pull request looks good
(what's this?)

Owner

talios commented May 1, 2012

Accepting pull requests is new for me as well:) Especially doing the back and forth, it makes me also think about the API more. As for the test, that one you've added is good, but I also more thinking about a test that gets the interface proxy and checks a method returns null when called via the proxy.

I'll pull this in but another test for the actual interface would be awesome :)

talios added a commit that referenced this pull request May 1, 2012

Merge pull request #26 from steveg103/develop
Allowing null properties in resources

@talios talios merged commit afc6d2b into talios:develop May 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment