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

[Dave Abrahams]

on Fri Jan 29 2016, Patrick Gili <swift-evolution at swift.org> wrote:

> My evaluation is inline below...

Thanks for your review, Patrick!

>> 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? 

Because it leads to a concise, readable summary that works well with
tools and often fully describes the API.  It is also easy to do.  It's
crucial that the most important part of the documentation is also easy
to produce, or people won't do it.

> I find sentence fragments often detract from clarity and concise
> nature, which can lead to confusion.

We have lots of examples that follow this style in the Swift standard
library.  Can you point at some that are confusing, unclear, or not
concise because they use sentence fragments?

> - 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.

Point taken.

> - 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?

Because it's not the declaration site that matters; it's the use site.

Just to take the first use-site I could find with GitHub search, imagine
the code at
https://github.com/phileggel/Hangman/blob/c14fbbfd06e58527832c0634785aee45cb9e5e13/SwiftHangman/View%20Controllers/HMViewController.swift#L11
said "add" instead of "addObserver".  Would it make sense?

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

Again, because it's the use-site that matters.  With "for keyPath", at
the use-site, you'd only see "for".

> - 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()? 

Because, among other things, "firstAndLast" incorrectly implies you only
get two parts back.

> Perhaps a better example is in order.

Suggestions welcomed.

> - 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. 

Yes, but we don't have the necessary language features
(e.g. https://github.com/apple/swift/blob/master/docs/proposals/Inplace.rst)
to do something like that today, so we need to use a naming convention
instead.

> 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. 

Example, please?  I have no idea what you might be referring to.

> 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.

I understand.  I don't think your request will get the traction you'd
like if you don't expose it somewhere other than in this thread.  We're
not considering new language rules here.

>> 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
>
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org
> https://lists.swift.org/mailman/listinfo/swift-evolution

-- 
-Dave