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

Adding configuration option to exclude null values from output #919

Closed
wants to merge 0 commits into from
Closed

Adding configuration option to exclude null values from output #919

wants to merge 0 commits into from

Conversation

navinpeiris
Copy link
Contributor

This adds a configuration options that allows the user to specify whether null values should appear in the resultant media type, and by default it's set to include null values so that existing behaviour isn't changed.

@joaomdmoura
Copy link
Member

@navinpeiris I know there is a lot of different opinions about the nulll return, a lot of mixed feelings 😝

We, as a team, are trying to push JSON-API forward, because we believe that it's an amazing convention that can help a lot of different people from different technologies. We just made it the default adapter, and the null return is one of its conventions.

So I'm not in favor of adding thins configuration. Of course we could still add it only to the JSON adapter, but I'm not sure if it worths.
cc @steveklabnik @guilleiguaran @kurko

@navinpeiris
Copy link
Contributor Author

Cool, thanks for that @joaomdmoura. Yeps, JSON-API does look like the bomb, but unfortunately we've got to stick with our current format for awhile.

Would this be much of an issue though considering it's provided as a configuration option for the user to chose which way they want to go, and by default it'll stay true to the spec as well?

I'm just trialing converting from ROAR to AMS to take advantage of fragmented caching, so this'll help making the responses a 100% compatible with the previous, but saying that, this is more of a nicety than a must, so feel free to close this off if it's not inline with where you peeps want to head

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

Successfully merging this pull request may close these issues.

3 participants