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

Add the ability to specify properties as optional #11

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ConnorAtherton
Copy link

This removes null fields when the objects are converted into the JSON representation for optional properties. This is useful for APIs where a property being optional either means a key is present in the request body or it isn't.

Let me know if I should add more test cases for this.

@phillbaker
Copy link
Contributor

Thanks for the PR @ConnorAtherton, this actually looks similar to #5. Some of the discussion points still apply - how would we tell the difference between intentionally null attributes and ones that are not set?

Before merging something like this, we'd also have to update the readme with an example of usage.

@ConnorAtherton
Copy link
Author

No problem @phillbaker. I should have looked through the open issues before adding this - it was something I had to add for a project I'm currently working on.

The user decides whether something is null be default or whether it is intentionally not set. I think it's important because people can use this library for other API's that they have no control over and so need as much flexibility as possible. This change doesn't remove how kartograph can represent objects now - by default null values will still show as they did before - it only adds another configuration option if it's something the user wants/needs.

Agreed, I think it's important to convey what 'optional :true` means to avoid confusion. Optional in this context means that the representation will only include the attribute if a value was given and that value was not nil. I will update the readme.

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.

2 participants