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

Experiment: Fix(?) keypaths and add decode overloads on NSDictionary #109

Closed
wants to merge 6 commits into from

Conversation

Anviking
Copy link
Owner

@Anviking Anviking commented Jul 29, 2016

Adds overloads of the following form:

extension NSDictionary {
    public func decode <A: Decodable>(_ keyPath: String...) throws -> A { ... }
}

Which enables the following usage:

let _: String = dict.decode("a", "b")
let _: String? = dict.decode("a", OptionalKey("b"))

With the side effects of making the OptionalKeyPath/KeyPath slightly more complex and with changes to default behaviours:

// OptionalKeys created from string literals have isRequired = true
let a1: OptionalKeyPath = ["a", "b"] // a.b

// OptionalKeys created with explicit initialiser have isRequired = false
let a2: OptionalKeyPath = ["a", OptionalKey("b")] // a.b?

I thought about adding a .optional property on String instead of OptionalKey("key"), but thought it was too vague.

This would fix #26, while maintaining the following features:

  • Keys that are allowed to be missing have to be explicitly (and individually) marked
  • No wrapping JSON type
  • Previous syntax still works

I'm not at all sold on this yet however. Some rambling thoughts:

  • The operator way and non-operator way cannot presumably live side by side forever
  • When dealing with dictionaries disguised as AnyObjects this adds verbosity
  • When dealing with optional keys this adds verbosity and removes the illusion that => works like a subscript, and the added ? in =>? works like in optional chaining. Though perhaps this is for the best because the implementation is made simpler, fixes one edge cases where the first key(s) are required where the latter are optional, and because this is how Decodable actually works.

Also in the future OptionalKey could add functionality to distinguish between accepting NSNull and accepting missing keys, a small but useful feature.

TODO:

  • Fix documentation
  • Think about it for a while

@Anviking
Copy link
Owner Author

Hmm, this also allows for collecting more metadata with #line e.t.c.

func decode<T>(keys: String..., line: Int = #line, file: String = #file) throws -> T

@voidref
Copy link
Collaborator

voidref commented Jul 29, 2016

If Swift is anything like C, the variadic has to be at the end of the parameter list. although it's treated like an array, so perhaps it doesn't? Can't seem to find anything in the doc about it except you can only have 1 variadic in a parameter list.

@voidref
Copy link
Collaborator

voidref commented Jul 29, 2016

Ok, quick playground test shows it's ok as long as the types are all the same, the compiler is ok.

Not sure, but as this is a template, err ... generic, we aren't expecting this to be overridden in subclasses, yes? Otherwise I am not sure subclassing isn't going to be ... weird, with the default parameters...

@Anviking
Copy link
Owner Author

Overriding it actually turned out to be impossible (currently), but you're right, it should be final.

@Anviking Anviking mentioned this pull request Jul 30, 2016
3 tasks
@Anviking
Copy link
Owner Author

Anviking commented Jul 30, 2016

Also, got this to work! (for separate pull request probably though)

public enum OptionalKey { // Better name needed?
    case optional(String)
    case required(String)
    case canBeMissing(String)
}
let dict =  NSDictionary()
let a: Int? = try! dict.decode("required", .required("alsoRequired"), .optional("yay"), .canBeMissing("more yay"))

@voidref
Copy link
Collaborator

voidref commented Jul 30, 2016

The OptionalKey thing is cool, but I agree it needs a better name.

The cases optional and canBeMissing are need some nomenclature work, as 'optional' is the same meaning as 'can be missing'.

Perhaps a name like MetaKey, SupplementalKey, or KeySpecifier?

I haven't been keeping up with this, so I am unsure about the meaning of the cases exactly, but perhaps it should be nullable, required, and optional?

The problem with that is that this naming confuses the meaning of optional. =(

What is the difference between the optional case and the canBeMissing case?

@Anviking
Copy link
Owner Author

@voidref I'm so unsure about everything, but to just answer your questions: optional would return nil when the object is NSNull, and canBeMissing would return nil instead of throwing .missingKey like the current =>? operator.

The reason for the OptionalKey though is that it's meant to be used in a keyPath where at least one key is optional / not required. I.e it can be misleading:

// Does not compile:
let a: Int = json.decode(.required("a")) // "a" becomes OptionalKey 

// Compiles but never returns nil:
let a: Int? = json.decode(.required("a")) // "a" becomes OptionalKey

// Compiles and works as expected:
let a: Int = json.decode("a") // "a" becomes String/KeyPath

@voidref
Copy link
Collaborator

voidref commented Jul 31, 2016

Given this answer, consider allowNull, allowMissing, and required for the cases.

Gah, naming, it's hard. I am not totally happy with those.

Perhaps it's because the underlying logic is too subtle. Would it be reasonable to wrap both the missing and the nsnull case into just one 'optional'?

@Anviking
Copy link
Owner Author

Closing this now even though some aspects of this have potential and could be revisited. If we're doing this it should be more like #97, which perhaps isn't that bad if you don't try to over-abuse the DecodingContext parameters.

@Anviking Anviking closed this Aug 17, 2016
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.

Refactor operators away
2 participants