Anviking / Decodable

[Probably deprecated] Swift 2/3 JSON unmarshalling done (more) right
MIT License
1.04k stars 73 forks source link

Rename closure functions #104

Closed Anviking closed 8 years ago

Anviking commented 8 years ago

Changes

decodeArray(_:) -> array(_:) catchNull(_:) -> optional(_:) decodeDictionary(_:elementDecodeClosure:) -> dictionary(key: element:)

Motivation

The main motivation for this is to encourage use of the functions that take and return decode functions, thereby encouraging not conforming to the Decodable protocol.

The places where they are intended to be used are for decode parameters, which is also how they are used internally in each overload. Since the parameter already is called decode, stripping the word from each call makes it more readable:

-return try parse(json, keyPath: keyPath, decode: catchNull(decodeArray(catchNull(decodeArray(A.decode)))))
+return try parse(json, keyPath: keyPath, decode: optional(array(optional(array(A.decode)))))

Furthermore catchNull is a misleading name that incorrectly implies that errors are caught (they did before though, and it was incorrect). Instead it works by checking if the object is NSNull.

decodeDictionarys only had a label for the value/element decode closures, that also was very long.

These new names could pollute the namespace since they are lowerspace variants of Swift types. However the files that import Decodable should be mostly model and networking code that is already very separated.

Alternatives

To document changes, motivations and give an opportunity for objections, even if I'm probably merging this right away.

voidref commented 8 years ago

Another option might be to call it xDecoder, as it's returning a closure that decodes a type, yes? This does nothing for readability.

It does seems a bit odd to call a global function that has a noun name that is the same for a built-in type.

Another thought is array(for:) which indicates better, I think, that you are generating a thing for another thing.

Anviking commented 8 years ago

I like it, but in that case, maybe

return try parse(json, keyPath: keyPath, decode: Optional.decoder(Array.decoder(Optional.decoder(Array.decoder(A.decode)))))
// or
return try parse(json, keyPath: keyPath, decode: Optional.decoder(for: Array.decoder(for: Optional.decoder(for: Array.decoder(for: A.decode)))))
voidref commented 8 years ago

Oh, the extensions on the type seem like a great idea, and a way to 'namespace' the functions.

Related, the 3rd parameter name, perhaps should be decoder: as it's passing a decoder in. The verb form decode: implies an additional side effect and that we are decode-ing something that's passed in.

This example does look a bit verbose, but I am hoping deeply nested calls aren't common.