API: adjust to swift 3 api design guide #31

bigyelow · 2017-01-16T06:40:59Z

除了 pr 中修改的，有几个剩下的但是觉得不太合理的没动：

下面的 protocol 方法命名不太清楚

  /**
   When the result returns, this method will be called.
   
   - parameter requestCode: The integer request code originally supplied to startControllerForResult(), allowing you to identify who this result came from.
   - parameter resultCode: The result code returned by the child conroller.
   - parameter intent: An Intent, which can return result data to the caller (various data can be attached to Intent "extras").
  */
  func onControllerResult(requestCode: Int, resultCode: FRDResultCode, data: FRDIntent)

看起来这像是一个 delegate 方法，1 是命名上或者注释都会让阅读者比较困扰，单从命名或者注释并不能理解这个方法是做什么的、什么时候会调用；2 是 delegate 方法一般会带上 delegate 调用者本身，比如 UIApplicationDelegate 中的方法

func application(UIApplication, didDecodeRestorableStateWith: NSCoder)

下面两个协议的命名有待商榷，首先名字就让人比较困惑...
FRDIntentForResultSendable
FRDIntentForResultReceivable

由于我对你的协议的设计思想不如你清晰，这几处你来看下吧。

lincode · 2017-01-16T06:51:53Z

在调用一个新的 view controller 以获取以一个结果时，使用这个协议。该方法用于获取结果。他并不需要知道谁返回了结果给它。只要取得结果即可。如果要把新的 view controller 以参数形式传回，那么就失去了解耦的意义。
func onControllerResult(requestCode: Int, resultCode: FRDResultCode, data: FRDIntent)

bigyelow · 2017-01-16T06:55:14Z

这样的命名并不符合英文语法？无法通顺的构成一个完整的句子。

lincode · 2017-01-16T06:56:34Z

那你提议一个吧。意思你已经懂了。

bigyelow · 2017-01-16T07:28:30Z

@lincode 根据下面两点，对 protocol 命名做了调整：

Protocols that describe what something is should read as nouns (e.g. Collection).
Protocols that describe a capability should be named using the suffixes able, ible, or ing (e.g. Equatable, ProgressReporting).

看下最后一个 commit 吧。

lincode · 2017-01-16T07:32:49Z

Protocols that describe a capability should be named using the suffixes able, ible, or ing (e.g. Equatable, ProgressReporting)
为什么不能根据第二句，使用 able 结尾呢？

bigyelow · 2017-01-16T07:37:30Z

嗯，单个协议而言，用 -able 也可以，但是考虑到是一堆协议：
对于 Intent 分为 sender, receipient;
对于 Result 分为 sender, resolver;

这样统一起来，都用名词，角色上更清晰。

lincode · 2017-01-16T07:39:28Z

其实只有两个协议：Sendable，和 Receivable，和它们的两个约束强一些的子协议。分别代表了 Intent 的发送方，和接收方。

bigyelow · 2017-01-16T07:40:03Z

另外，我觉得从语义上，作为名词，表达一种角色，更自然。
下面的代码

return routeManager.register(url, clazz: clazz as! FRDIntentRecipient.Type)
as FRDIntentReceipient 比 as FRDIntentReceivable 更自然

lincode · 2017-01-16T07:40:34Z

我建议这两个协议名不用修改了：
1 改名之后并没有带来清晰性上的改观；
2 而这个改了，使用该库的代码需要跟着改。

lincode · 2017-01-16T07:43:42Z

这里是名词清楚，还是形容词清楚。说实话，都是语义学上的争论。
如果没有明显的清晰性上的提升，还是建议保留。因为，这里的改动带来了使用该库的代码需要跟着做修改。

bigyelow · 2017-01-16T07:44:07Z

ok，那我把最后一个 commit 去掉吧

bigyelow · 2017-01-16T07:45:38Z

@lincode 去掉了对协议的改动

lincode · 2017-01-16T07:48:48Z

onControllerResult 这个方法呢？有什么建议？

bigyelow · 2017-01-16T07:55:48Z

本来打算这么改，应该更清晰

@objc public protocol FRDResultResolver {
  func receiveResult(withRequestCode requestCode: Int, resultCode: FRDResultCode, data: FRDIntent)

}

lincode · 2017-01-16T09:18:07Z

onControllerResult 来源于 Android API。可以叫 controllerDidReturn ?

bigyelow · 2017-01-16T09:20:27Z

可以，我改下吧。 onControllerResult 估计是 android 的 api 风格，iOS 很少这样命名。

lincode · 2017-01-16T09:21:13Z

记得 Demo 里用到了。

bigyelow · 2017-01-16T09:23:55Z

@lincode done.

lincode · 2017-01-16T09:31:01Z

FRDIntent/Source/Intent/IntentForResult.swift

@@ -34,7 +34,7 @@ import Foundation
   - parameter resultCode: The result code returned by the child conroller.
   - parameter intent: An Intent, which can return result data to the caller (various data can be attached to Intent "extras").
  */
-  func onControllerResult(requestCode: Int, resultCode: FRDResultCode, data: FRDIntent)
+  func controllerDidReture(withReqeustCode requestCode: Int, resultCode: FRDResultCode, data: FRDIntent)


typo: Return

lincode · 2017-01-17T02:39:00Z

放几天，让大家学习一下，再合吧。

bigyelow · 2017-01-17T02:50:34Z

@lincode 好的。

daydayfree · 2017-01-17T03:40:19Z

FRDIntent/Source/Core/Trie.swift

@@ -67,7 +67,7 @@ final class Trie<T> {
      if let child = currentNode.children[path] {
        currentNode = child
      } else {
-        currentNode.addChild(key: path, value: value)
+        currentNode.addChild(withKey: path, value: value)


这个 withKey 与 value 不能对应，是不是需要改成 addChildWith(key:String, value: String) ?

带介词的都作为 argument label，没有接口会把 with 放在名字里. 这些规则都是明确的，不是凭感觉或者想象，最好再多看下 https://swift.org/documentation/api-design-guidelines/#promote-clear-usage 或者我总结的 keynote http://wiki.intra.douban.com/tech/iOS

附带苹果 api 实例：

presentOpenInMenu(from:animated:) presentController(withNames:contexts:)

@bigyelow 没凭感觉 😢

@bigyelow @daydayfree 我读了一下文档，这里似乎大麦的感觉也有道理：

An exception arises when the first two arguments represent parts of a single abstraction.
In such cases, begin the argument label after the preposition, to keep the abstraction clear.

a.moveTo(x: b, y: c) a.fadeFrom(red: b, green: c, blue: d)

Key 和 value 共同构成了 Child。和 move 到一个点是相同的意思。

daydayfree · 2017-01-17T03:50:08Z

FRDIntent/Source/Core/RouteManager.swift

@@ -29,13 +29,13 @@ class RouteManager {
   - parameter url: The path for search the storage position.
   - parameter clazz: The clazz to be saved.
   */
-  @discardableResult func register(url: URL, clazz: FRDIntentReceivable.Type) -> Bool {
+  @discardableResult func register(_ url: URL, clazz: FRDIntentReceivable.Type) -> Bool {


这里是不是 register(withURL:URL, forClazz:Type) 更合适一点？

逻辑反了，逻辑上 key = url , object = clazz。一般的写法是 setObject(_:forKey:)
但是这里相当于 set(key:object:)

附带 apple api

setSpecific(key:value:)

daydayfree · 2017-01-17T03:52:25Z

上面说的是不是跟这里一样？

bigyelow · 2017-01-17T03:54:58Z

不一样，key value 之间的关系和 x, y, z 之间的关系是不同的。

daydayfree · 2017-01-17T04:10:15Z

这里是怎么做区分的？

bigyelow · 2017-01-17T04:11:32Z

你指哪两种情况？

daydayfree · 2017-01-17T04:12:09Z

不一样，key value 之间的关系和 x, y, z 之间的关系是不同的

bigyelow · 2017-01-17T04:17:33Z

这，从语义上吧，下面的解释感觉说得挺清楚了啊...

An exception arises when the first two arguments represent parts of a single abstraction.

key->value 表示的映射；x, y, z 分别只是某个概念的组成部分，不存在 x->y 的映射.

daydayfree · 2017-01-17T04:33:43Z

这也算个人习惯吧，我倾向你这种写法，介词放在名字后面不太好看。不过跟映射应该没啥关系，
func addAttribute(_ name: String, value value: AnyObject, range aRange: NSRange) 这里的参数也是同级的，所以我觉得保持 argument label 形式一致就好。

有介词就都有介词，除非第一个参数的 label 挪到了方法名之中。

bigyelow · 2017-01-17T05:45:49Z

这不是个人习惯啊，是规则。

when the first two arguments represent parts of a single abstraction.

a.moveTo(x: b, y: c)

这里为什么要把介词抽出来，不是说的参数同级，参数之间没有同不同级的概念。上面的英文指的是 x, y 共同构成一个抽象的概念：坐标。坐标= (x, y)。

而下面的 name, value, range 他们没有共同构成什么概念，所以这是两件事情。
func addAttribute(_ name: String, value value: AnyObject, range aRange: NSRange)

bigyelow · 2017-01-17T05:47:38Z

我提到映射，只是一个例子，为了说明两者有差别

daydayfree · 2017-01-17T05:52:00Z

按你说的 currentNode.addChild(withKey: path, value: value) key + value = Node 吧？

daydayfree · 2017-01-17T05:54:01Z

你可以尝试着理解一下我的想法， func addAttribute(_ name: String, value value: AnyObject, range aRange: NSRange)
name + value + range = attribute

daydayfree · 2017-01-17T05:55:52Z

其实苹果 api 里面这两种都有

bigyelow · 2017-01-17T05:56:57Z

坐标=(x, y) ，color=(red, green, blue) 这种概念和 attribute = name + value + range 是不同的，怎么用语言表达出来，我说不出来。
api 是 func addAttribute(_ name: String, value value: AnyObject, range aRange: NSRange) 而不是 func addAttributeWith()
正说明了 name + value + range != attribute 。

bigyelow · 2017-01-17T05:57:06Z

你举个例子呢？

daydayfree · 2017-01-17T05:59:49Z

说反了吧，因为 name + value + range = attribute 所以没有用 func addAttributeWith()

daydayfree · 2017-01-17T06:00:23Z

🙈 我已经绕晕了

bigyelow · 2017-01-17T06:01:02Z

没反

daydayfree · 2017-01-17T06:03:37Z

=。=

daydayfree · 2017-01-17T06:11:43Z

我再学习下

bigyelow · 2017-01-17T06:13:42Z

想到一个差别坐标=(x, y)， color = (red, blue, green) 中的 x y, red blue green 都是一样的类型，表示一样的概念。
name value range 不是一样的类型也不表示一样的概念。

calvingit · 2017-01-18T02:44:43Z

你们内部用到这个东西没有啊？改接口名字好麻烦啊

bigyelow · 2017-01-18T02:54:06Z

@calvingit 我们内部会用的。改接口名字还好，趁这个项目还在初期，以后来改就更麻烦了。

bigyelow · 2017-01-23T08:12:31Z

@lincode 可以合了？

lincode · 2017-01-23T09:39:05Z

@bigyelow 我明天合。

lincode · 2017-01-25T08:50:02Z

@daydayfree @bigyelow addChild 这个函数的意思是：在一颗 Trie 树中，添加一个节点。这个节点需要一个 key 和一个 value。key 和 value 共同组成了这个节点。

huangduyu added 7 commits January 16, 2017 12:21

API: addjust Trie api

566a7bc

API: addjust FRDURLRoutes api

0a83722

API: addjust FRDControllerDisplay api

e61a85a

API: addjust FRDIntent api

5f9ef07

API: addjust FRDControllerManager api

4ba8ea8

API: addjust FRDIntentReceivable api

b042ad4

API: addjust FRDURLRoutes api

e453d73

bigyelow force-pushed the master branch from 96cd271 to e453d73 Compare January 16, 2017 07:45

API: addjust IntentForResult API

d91044b

lincode reviewed Jan 17, 2017

View reviewed changes

review-feedback: fix type and warning

a9f420a

daydayfree reviewed Jan 17, 2017

View reviewed changes

lincode merged commit 365f6cc into douban:master Jan 25, 2017

lincode changed the title ~~API: addjust to swift 3 api design guide~~ API: adjust to swift 3 api design guide Jan 25, 2017

API: adjust to swift 3 api design guide #31

API: adjust to swift 3 api design guide #31

Conversation

bigyelow commented Jan 16, 2017 • edited Loading

lincode commented Jan 16, 2017 • edited Loading

bigyelow commented Jan 16, 2017

lincode commented Jan 16, 2017

bigyelow commented Jan 16, 2017

lincode commented Jan 16, 2017 • edited Loading

bigyelow commented Jan 16, 2017

lincode commented Jan 16, 2017 • edited Loading

bigyelow commented Jan 16, 2017 • edited Loading

lincode commented Jan 16, 2017 • edited Loading

lincode commented Jan 16, 2017 • edited Loading

bigyelow commented Jan 16, 2017

bigyelow commented Jan 16, 2017

lincode commented Jan 16, 2017

bigyelow commented Jan 16, 2017

lincode commented Jan 16, 2017 • edited Loading

bigyelow commented Jan 16, 2017 • edited Loading

lincode commented Jan 16, 2017

bigyelow commented Jan 16, 2017

lincode Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

lincode commented Jan 17, 2017

bigyelow commented Jan 17, 2017

daydayfree Jan 17, 2017

Choose a reason for hiding this comment

bigyelow Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

daydayfree Jan 17, 2017

Choose a reason for hiding this comment

lincode Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

daydayfree Jan 17, 2017

Choose a reason for hiding this comment

bigyelow Jan 17, 2017

Choose a reason for hiding this comment

daydayfree commented Jan 17, 2017

bigyelow commented Jan 17, 2017

daydayfree commented Jan 17, 2017

bigyelow commented Jan 17, 2017

daydayfree commented Jan 17, 2017

bigyelow commented Jan 17, 2017 • edited Loading

daydayfree commented Jan 17, 2017

bigyelow commented Jan 17, 2017 • edited Loading

bigyelow commented Jan 17, 2017 • edited Loading

daydayfree commented Jan 17, 2017

daydayfree commented Jan 17, 2017

daydayfree commented Jan 17, 2017

bigyelow commented Jan 17, 2017

bigyelow commented Jan 17, 2017

daydayfree commented Jan 17, 2017

daydayfree commented Jan 17, 2017 • edited Loading

bigyelow commented Jan 17, 2017

daydayfree commented Jan 17, 2017

daydayfree commented Jan 17, 2017

bigyelow commented Jan 17, 2017

calvingit commented Jan 18, 2017

bigyelow commented Jan 18, 2017

bigyelow commented Jan 23, 2017

lincode commented Jan 23, 2017

lincode commented Jan 25, 2017

bigyelow commented Jan 16, 2017 •

edited

Loading

lincode commented Jan 16, 2017 •

edited

Loading

lincode commented Jan 16, 2017 •

edited

Loading

lincode commented Jan 16, 2017 •

edited

Loading

bigyelow commented Jan 16, 2017 •

edited

Loading

lincode commented Jan 16, 2017 •

edited

Loading

lincode commented Jan 16, 2017 •

edited

Loading

lincode commented Jan 16, 2017 •

edited

Loading

bigyelow commented Jan 16, 2017 •

edited

Loading

lincode Jan 16, 2017 •

edited

Loading

bigyelow Jan 17, 2017 •

edited

Loading

lincode Jan 25, 2017 •

edited

Loading

bigyelow commented Jan 17, 2017 •

edited

Loading

bigyelow commented Jan 17, 2017 •

edited

Loading

bigyelow commented Jan 17, 2017 •

edited

Loading

daydayfree commented Jan 17, 2017 •

edited

Loading