Skip to content

Add include_nil property option#2

Closed
bernd wants to merge 2 commits into
trailblazer:masterfrom
bernd:include_nil-property-option
Closed

Add include_nil property option#2
bernd wants to merge 2 commits into
trailblazer:masterfrom
bernd:include_nil-property-option

Conversation

@bernd

@bernd bernd commented Jan 3, 2012

Copy link
Copy Markdown

Implements a :include_nil option for properties as mentioned in trailblazer/roar#7.

@apotonick

Copy link
Copy Markdown
Member

Should we call that option include_nil or include_if_nil? Thanks, Bernd!

@bernd

bernd commented Jan 4, 2012

Copy link
Copy Markdown
Author

I kinda like include_nil because include_if_nil sounds weird to me. But in the end, I don't really care. ;)

Nick, do you like include_if_nil better? I'd update the patch then.

@apotonick

Copy link
Copy Markdown
Member

Bernd- here's what mee understands when reading both versions:

  • include_nil include nil into the property (?)
  • include_if_nil include the property even if it's nil

@docwhat

docwhat commented Jan 4, 2012

Copy link
Copy Markdown

I prefer include_if_nil or maybe even include_nil_values

@cowboyd

cowboyd commented Jan 4, 2012

Copy link
Copy Markdown

Thinking off the cuff here, but how about represent_nil?

this most closely describes what you're doing, right? Also, it leaves the door open to configure its representation

represent_nil :as => false

@apotonick

Copy link
Copy Markdown
Member

@cowboyd Do you mean a global switch to decide if nil properties should be included or not? Cause the problem here is how we define per property whether or not to include it in the doc if it's nil.

@bernd

bernd commented Jan 4, 2012

Copy link
Copy Markdown
Author

I like @cowboyd's version. I guess it would look like this.

property :beer, :represent_nil => true
represent_nil :as => false

@apotonick

Copy link
Copy Markdown
Member

Ok, one global configuration "how to convert nil values" and a :represent_nil option. How to set the conversion per property, then?

I have to refactor Definition to make it easier to add custom options - behavior like this can then be added as module and doesn't have to be hard-wired in the core. Cool?

@bernd

bernd commented Jan 6, 2012

Copy link
Copy Markdown
Author

@apotonick Yes, sounds good!

@jwkoelewijn

Copy link
Copy Markdown

Is there a current status for this?

For me the problem really lies in the parsing, when using :class option and supplying nil's (or not supplying values at all) the parsing breaks ( undefined method `[]' for nil:NilClass ), unless when using a :if => lambda{ !property.is_nil? }, but maybe this is a whole different issue?

@apotonick

Copy link
Copy Markdown
Member

@jwkoelewijn Can you show a quick example how to provoke that bug? I'm working on this as I type but I'm not sure if that is directly connected to your issue.

@jwkoelewijn

Copy link
Copy Markdown

My problem seemed to originate in the Roar gem, so created a pull request with a fix for Roar

@apotonick

Copy link
Copy Markdown
Member

The :represent_nil => true feature is now available in 1.2.0 21ce092

@apotonick apotonick closed this May 31, 2012
@apotonick

Copy link
Copy Markdown
Member

This option has been renamed to :render_nil and not been deprecated in 1.2.1 since I was too lazy.

@cowboyd

cowboyd commented Aug 7, 2012

Copy link
Copy Markdown

This is still problematic for me in the case where the value could be populated with a complex object (as opposed to a simple property)

property :image, :extend => ImageRepresenter, :render_nil => true

This completely barfs for me because nil is extended with ImageRepresenter and then it tries to actually render . What I think most folks want with :render_nil is to have some placeholder value so that the key appears in the output, but not to actually attempt to render nil through the normal codepath.

@apotonick apotonick reopened this Aug 7, 2012
@apotonick

Copy link
Copy Markdown
Member

This is a bug, the nil value shouldn't be extended when render_nil is set. Maybe we can fix it over beers at the LSRC 6, @cowboyd ;-)

@apotonick

Copy link
Copy Markdown
Member

The nil bug is fixed in 1.2.4.

@apotonick apotonick closed this Aug 25, 2012
hcnguyen5 pushed a commit to tastyworks/representable that referenced this pull request Jan 4, 2017
revert bundler to just rspec and rubocop
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.

5 participants