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

Rename closure functions #104

Merged
merged 1 commit into from
Jul 13, 2016
Merged

Rename closure functions #104

merged 1 commit into from
Jul 13, 2016

Conversation

Anviking
Copy link
Owner

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

  • optional(of: array(of: T.decode))?
  • Keep decode prefix
  • ?

Why did I write this?

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

@Anviking Anviking merged commit 88a4e1e into master Jul 13, 2016
@voidref
Copy link
Collaborator

voidref commented Jul 13, 2016

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
Copy link
Owner Author

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
Copy link
Collaborator

voidref commented Jul 14, 2016

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.

@Anviking Anviking mentioned this pull request Jul 29, 2016
@Anviking Anviking deleted the renaming branch September 2, 2016 06:43
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