Siesta API guidelines audit.md

Proposed changes to Siesta’s API before it hits the 1.0 freeze. Please post your feedback on this document to the accompanying Github issue.

Argument names

“Prefer to follow the language’s defaults for the presence of argument labels”

• These changes seem good:

  • Entity.init(_:_:) → init(response:content:)
  • Error.init(_:_:_:userMessage:) → init(response:content:cause:userMessage:)

• The guidelines suggest resourceWithURL(_:) instead of Service.resource(url:). However, after intensive discussion, (1) everyone preferred the original approach of labeling the parameter instead of appending WithFirstParameterDescription to the method name; however, (2) we determined that using the word “absolute” would help prevent misunderstanding about the nature of this method, and its difference from the flavor that takes a subath of baseURL:

  • Service.resource(url:) → resource(absoluteURL:)
  • Service.resource(_:) → same

• The guidelines would have us do addObserverWithOwner(foo) { … }. However, this is not only less readable than the current approach, but leads to the unfortunate juxtaposition of addObserver(foo, owner: bar) vs. addObserverWithOwner(bar) {...}, which incorrectly implies that “observer with owner” is a different thing than just “observer.”

  • Resource.addObserver(owner:closure:) → same

• This one is problematic:

  • TypedContentAccessors.contentAsType(ifNone:) → ???

  Without the ifNone: label, a call sounds as if the argument indicates the type — which is true, sort of, but confusing:

  resource.contentAsType(placeholderImage)
  

  The more explicit method name is unwieldy:

  resource.contentAsTypeWithDefault(placeholderImage)
  

  To my eyes, Siesta’s existing guideline-breaking alternative reads well:

  resource.contentAsType(ifNone: placeholderImage)
  

  However, I like this alternativealmost equally well, and it’s strongly preferred by Radek and Ray, so we’ll go with it:

  resource.typedContent(ifNone: placeholderImage)
  

Boolean properties

“Non-mutating Boolean methods and properties should read as assertions about the receiver”

• All seem clear:

  • Resource.loading → isLoading
  • Resource.requesting → isRequesting
  • Request.completed → isCompleted

Mutating methods

“Mutating methods should read as imperative verb phrases”

• These are clear improvements:

  • Resource.localDataOverride(_:) → overrideLocalData(_:)
  • Resource.localContentOverride(_:) → overrideLocalContent(_:)

• For this one, there’s not an obvious imperative verb rephrasing that isn’t either too verbose or less clear:

  • Configuration.beforeStartingRequest(_:) → same

  The guideline-conforming executeBeforeStartingEachRequest or addPreRequestHook don’t read as well.

• The authors appear not to have had in mind the common idiom of naming notification methods as past tense phrases:

  • ResourceObserver.resourceChanged(...) etc. → same

  The recommendation would want respondToResourceChanged, which does not seem like an improvement.

• Adding request hooks mutates the request, so the current hook names such as completion(_:) and success(_:) violate this recommendation. The guidelines would appear to recommend something like Request.addCompletionHook(_:), addSuccessHook(_:) and so forth. However, this runs contrary to the well-established fluent style of such hooks, and hurts readability. Still, the hook names as they stand — especially progress(_:) — are somewhat ambiguous. I therefore propose an on prefix:

  • Request.completion(_:) → onCompletion(_:)
  • Request.success(_:) → onSuccess(_:)
  • Request.newData(_:) → onNewData(_:)
  • Request.notModified(_:) → onNotModified(_:)
  • Request.failure(_:) → onFailure(_:)
  • Request.progress(_:) → onProgress(_:)

Non-mutating methods

“non-mutating methods should read as noun phrases”

• These methods do not mutate the receiver, but clearly read best as imperative verbs nonetheless:

  • ResponseTransformer.process(_:) → same
  • NetworkingProvider.startRequest(_:completion:) → same

• This one also runs afoul of “Avoid abbreviations”:

  • Resource.withParam(_:) → same

  The recommendation-friendly name would be resourceWithParameter. However, given frequent repetition of this method name in a call chain, the resulting verbosity would impair readability. The existing name is perfectly clear.

Nouns everywhere

“The names of other types, properties, variables, and constants should read as nouns”

• Although some of them read as past participles or even compressed sentences, not nouns, they do not benefit from this recommendation:

  • ResourceEvent “-ed” members: ObserverAdded, Requested, etc. → same

Needless words

“Omit words that merely repeat type information”

• I liked baseURL → base at first, but subsequent discussion convinced me that “base” alone is ambiguous and potentially misleading:

  • Service.baseURL → same

• Here “optional" repeats type information. However, overloading return type alone causes confusing compiler errors:

  • Resource.optionalRelative(_:) →  same