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

Added option to encode null values #26

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pfandrade
Copy link

There are some situations where not encoding a property, or encoding it with a null value will result in different behaviours.

So I've added the ability to do this while trying to keep backward compatibility.

@matthewcheok
Copy link
Owner

Thanks for the PR! It looks like you thought through this. I'm just concerned this would be make it ambiguous which variant of toJSON() to override.

  1. What do you think about introducing a parameter on the encode method for optionals:
public func encode<Enum: RawRepresentable>(value: Enum?, key: String, includeNull: Bool = false) throws
  1. The user decides if they want to opt in to this feature via overriding toJSON().

I reckon we can best solve this problem is by eventually supporting a partially specialised JSONTransformer<AnyObject, U?> but I don't believe Swift currently supports this.

@matthewcheok
Copy link
Owner

I should say the redundant method was because the compiler couldn't find the generic version when we use the default implementation that generates toJSON() automatically using mirrors.

This might be fixed the latest version of Swift but I've yet to test it.

Paulo Andrade added 2 commits January 15, 2016 17:38
…nd JSONDictionary to be consistent with the visibility of these types

This also allows for clients (that include the code not the framework) to use the JSONArray and JSONDictionary helper extensions
This should probably be the default...
@pfandrade
Copy link
Author

You're right about having two toJSON() methods. I don't like it either, the only reason I maintained the initial one was for backward compatibility.

Your proposal works as well, and is easy enough to implement. My main issues with it is that it spreads a "setting" throughout the code.

For cases where the developer always wants to encode nulls, having to pass that extra parameter in every single encode call will seem like an unnecessary burden.

For cases where he actually wants to choose wether to encode nulls or not depending on the context, he must add logic to his model to change that setting in all encode calls.

I also noticed that empty collections are not serialized, which should also be an option.

So here's what I'm thinking... I'll make toJSON() receive an OptionSet with both settings (encode nulls, and encode empty collection) and make it the single method defined in JSONEncodable. This has the advantage of easily allowing future options (say pretty printing for example) and avoids any ambiguity, but would be a backward incompatible change.

@pfandrade
Copy link
Author

Actually, the more I think about it, the more I feel like not encoding empty collections is actually a bug, and it should not even be an option. It's like not encoding an empty string.

… received a set of options for encoding

- JSONEncoder.create method also has a mandatory options parameter
- Empty collections are now always encoded
@matthewcheok
Copy link
Owner

Thanks for thinking about this more. I see your point. My final suggestion is to leave the toJSON() interface unchanged and allow for settings in JSONEncoder because it's somewhat clunky to have toJSON([]) in the general case and as you've mentioned, it's not a setting that's likely to change once your model is declared. While it's a breaking change, consumers don't have to update their code in too many places.

func toJSON() throws -> AnyObject {
  return try JSONEncoder.create([.EncodeNulls]) { (encoder) -> Void in
    ...
  }
}

Please also squash your commits so it's its easier to follow the changes. Thanks!

Conflicts:
	JSONCodableTests/RegularTests.swift
	JSONCodableTests/User.swift
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