[swift-evolution] [swift-evolution-announce] [Review] SE-0023 API Design Guidelines

[Patrick Gili]

My evaluation is inline below...

> On Jan 22, 2016, at 4:02 PM, Douglas Gregor <dgregor at apple.com> wrote:
> 
> Hello Swift community,
> 
> The review of SE-0023"API Design Guidelines" begins now and runs through January 31, 2016. The proposal is available here:
> 
> https://github.com/apple/swift-evolution/blob/master/proposals/0023-api-guidelines.md <https://github.com/apple/swift-evolution/blob/master/proposals/0023-api-guidelines.md>
> Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
> 
> https://lists.swift.org/mailman/listinfo/swift-evolution <https://lists.swift.org/mailman/listinfo/swift-evolution>
> or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
> 
> Proposal link:
> 
> https://github.com/apple/swift-evolution/blob/master/proposals/0023-api-guidelines.md <https://github.com/apple/swift-evolution/blob/master/proposals/0023-api-guidelines.md>
> Reply text
> 
> Other replies
>  <https://github.com/apple/swift-evolution#what-goes-into-a-review-1>What goes into a review?
> 
> The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
> 
> What is your evaluation of the proposal?
Overall, I like the proposal. It provides sound guidance that can lead to consistent code in practice. However, there are some issues and concerns I have:

- Under "Fundamentals", there is a bullet called "Write a documentation comment...". Under this bullet, this is another bullet called "Use a single sentence fragment...". Why? I find sentence fragments often detract from clarity and concise nature, which can lead to confusion.

- Under "Naming", there is a bullet called "Omit Needless Words". This bullet states, "Occasionally, repeating information is necessary to avoid ambiguity..." It would be useful to provide an example that the reader can use to disambiguate this use case.

- Under "Naming", there is a bullet called "Compensate for Weak Type Information...". The example provided is confusing. First, it contradicts the guidance relating to omitting needless words. It suggests that 

func add(observer: NSObject, for keyPath: String)

becomes

func addObserver(_ observer: NSObject, forKeyPath: String)

This results in "Observer" followed by "observer". Why is this more clear?

In addition, I don't understand why it collapsed "for keyPath" to "forKeyPath". Perhaps an explanation would clarify?

- Under "Naming", there is a bullet called "Uses of non-mutating methods should read as noun phrases...". This bullet provides an example of an exception. Why would calling this method firstAndLast() have less clarity than split()? Perhaps a better example is in order.

- Under "Naming", there is a bullet called "When a mutating method is described by a very, name its non-mutating counterpart...". On the surface this appears to provide good guidance, I think in practice it becomes difficult and doesn't necessarily provide the desired result. I think Ruby provides a better example of how the desired result is very clear. In Ruby, the name of a mutating method is designated by an exclamation point at the end of the name. For example, myString.reverse() returns the value of the reversed string, while myString.reverse!() mutates myString with the reversed value. I'm not necessarily proposing that Swift use the exclamation point for this purpose (as Swift already uses this force unwrapping), so much as I'm proposing that you investigate how other languages clearly disambiguate non-mutating and mutating methods.

- Under "Conventions", there is a bullet called "Methods can share a base name when they share the same basic meaning...". There are some examples when this convention does not apply. I think it would be helpful to illustrate to the reader how to address these exceptions (i.e., do this, instead of that).

- Under "Conventions", there is a bullet called "Prefer to follow the language's defaults for the presence of argument labels". Be aware that the current compiler issues the warning when the first argument label is "_", "Extraneous '_' in parameter: <parameter> has no keyword argument name". I would either like the compiler to not issue a warning for this use case, or at least give the developer the means to disable this warning.

> Is the problem being addressed significant enough to warrant a change to Swift?
I think publishing a clear set of guidelines is absolutely necessary.

> Does this proposal fit well with the feel and direction of Swift?
Absolutely. Great job!

> If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
I think all serious languages have a document that specifies a set of guidelines with the intent of bringing consistency in the use of the language. Languages that do not drive a set of best practices tend to suffer, at least form a readability perspective. For as much as I like Ruby, it lacks such a set of guidelines, which has resulted in the community writing code that lacks the consistency that makes it easy to read others' code.

> How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
As always, I read the proposal multiple times, which included reading through the guidelines multiple times, sleeping on it, pondering it, and finally writing my evaluation.

> More information about the Swift evolution process is available at
> 
> https://github.com/apple/swift-evolution/blob/master/process.md <https://github.com/apple/swift-evolution/blob/master/process.md>
> Thank you,
> 
> -Doug Gregor
> 
> Review Manager
> 
> _______________________________________________
> swift-evolution-announce mailing list
> swift-evolution-announce at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution-announce

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20160129/1d51ed64/attachment.html>