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

prevent object representations from including properties without values ... #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

activefx
Copy link

...(fixes #4)

@bobbytables
Copy link
Contributor

Thanks for the PR.

This is scares me because sometimes you want false as a value to be rendered. This would prevent that, the same with null.

I'd probably opt to have something like

property :name, omit_nil: true

@activefx
Copy link
Author

Great point, completely forgot about false. Ill update the request.

@bobbytables
Copy link
Contributor

Do you have any thoughts on omit_nil: true as an option while defining properties?

@activefx
Copy link
Author

That would be the quickest fix. The other thought I had was differentiating between optional and required properties, where optional properties with nil values would not be included and required properties would always be set to null. But that gets complex quickly when working with the scopes, and again there might be an edge case.

Another possibility could be to add an optional reduce / finalize function that takes place after the representation_for mapping.

kartograph do
  mapping User

  property :name, scopes: :read
  property :avatar, scopes: [:read, :create] 

  reduce { |hash| hash.delete_if { |k, v| v.nil? } }, scopes: :create
end

@bobbytables
Copy link
Contributor

The finalize mechanism is an interesting option. I'm mostly concerned with it being viewed as an option to use callbacks on serialization. Which is something I'd really like to avoid. I think the use-case for giving omit_nil: true is great.

@activefx
Copy link
Author

What are your thoughts on allow_nil instead of omit_nil? On API put requests that only update a select few attributes, passing the rest of the attributes as nil values may cause them to become unset. By default, I suggest that nil values are not included in the representation.

@davidcelis
Copy link

I would personally hesitate to call this an issue. As Kartograph's stated intention is to be used for APIs, the mappings it returns should always have the same structure. This means values should be shown as null as opposed to removed entirely. While I understand your use case here, I would urge you not to make properties in your API responses optional, instead letting clients handle null values

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.

Artist draws values for all properties, even if they are not set
3 participants