Skip to content

Enable flag on exposures to optionally exlcude nil attributes. #10

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

Closed
wants to merge 1 commit into from

Conversation

idyll
Copy link
Contributor

@idyll idyll commented Mar 29, 2013

For certain APIs I find that it is best for the client if the API hide's attributes that are nil.

Instead of handling this in each serializer (json, plist, xml) I am looking for options to pull control over this into the entity.

@idyll
Copy link
Contributor Author

idyll commented Mar 29, 2013

@dblock or @mbleigh can you give me your thoughts on this. I am trying to think of an easy way to check the value of an attribute and hide it if it is nil.

I am really undecided at this point if this should be an option at the entity level or at the individual exposure level.

Either way, I think conditions_met should accept the attribute. Eventually I think we should pass it to the proc with the object but I think this is a breaking change because existing procs won't be able to handle it.

@SegFaultAX
Copy link
Contributor

I think this is a pretty cool feature, and I definitely think it should be on a per-exposure basis. If you have a host of nullable exposures, you can use the new #with_options method that @mbleigh added recently to share that configuration easily while keeping the entity definition DRY. +1

@mbleigh
Copy link
Contributor

mbleigh commented Mar 29, 2013

I like the feature, and also agree that it should be per-exposure. There are some attributes that you may want to expose as null even if others aren't. Make it per-exposure and I'd say it's good to merge!

@mbleigh
Copy link
Contributor

mbleigh commented Mar 30, 2013

Note I'm not 100% if the current implementation is per-exposure, but it seems like the specs at least could use a bit more descriptive of names.

@idyll
Copy link
Contributor Author

idyll commented Mar 30, 2013

I agree. The naming is bad. This was intended to talk about, I wasn't planning to merge until I cleanup the specs and make sure i've covered what I need to.

The current implementation should be per-exposure.

@dblock
Copy link
Member

dblock commented Aug 5, 2013

Bump. What do we want to do about this one? If anything it needs a line in CHANGELOG :)

@idyll
Copy link
Contributor Author

idyll commented Aug 6, 2013

I need to fix it. I am just getting back into this now after some craziness.

@dblock
Copy link
Member

dblock commented Sep 16, 2013

In the spirit of staying positive, I think this should be renamed to expose_nil, which would default to true.

Note that exclude_nil can be implemented today with an if.

@idyll
Copy link
Contributor Author

idyll commented Sep 17, 2013

Okay, I should have time towards the end of the week to fix this. I am going to fix the implementation and I like the expose_nil wording.

@xurde
Copy link

xurde commented Feb 22, 2014

So was this finally implemented?
I cant see anything about this on the Docs.

Thanks.

@dblock
Copy link
Member

dblock commented Feb 22, 2014

This hasn't been merged, see discussion above. Need more work (from @idyll or others).

@xurde
Copy link

xurde commented Feb 23, 2014

Thanks @dblock
In the meanwhile just found this workaround that needs to be applied to every field.

http://stackoverflow.com/questions/21950613/grape-entities-conditional-expose-if-field-is-not-nil

Hope it help.
Cheers

@idyll
Copy link
Contributor Author

idyll commented Mar 28, 2014

I am closing this and will reopen a new one with my changes,

@idyll idyll closed this Mar 28, 2014
@aj0strow
Copy link
Contributor

Was this ever re-opened in another issue? Would be a nice feature for with_options with infrequent relations.

with_options(expose_nil: false) do
  expose :snow_route
  expose :temporary_route
  expose :construction_warning
end

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.

6 participants