YUKAI / konashi-ios-sdk

konashi iOS SDK
http://konashi.ux-xu.com
Apache License 2.0
89 stars 37 forks source link

リファクタリング #23

Closed 0x0c closed 10 years ago

0x0c commented 10 years ago

以下の点の修正を行いました ・workspaceの採用 ・メソッド名及び定数の命名規則の修正/統一 ・UIに関するコードの分離 ・シングルトンの廃止 ・blocksの採用 ・複数台のKonashiを同時接続する機能の実装

変更が多岐にわたり、規模が大きくなってしまいましたがよろしくお願いいたします

mash commented 10 years ago

konashiの1ファンとしてコメントします。 自分は正式にメンテナではないので気にしないでいただいてもよいです。

最初に申し上げておきますがいくつか異論はあるものの基本的にはpullrequestの変更には賛成です。 自分もプライベートで、いかにkonashi-ios-sdkがiOSエンジニアにとって親しみにくいつくりか、ということについて指摘させてもらったことがあります(笑

ですが、既に世の中に出ている、そこそこユーザーがいるライブラリの作者として、このpullrequestを簡単にマージできないのはよくわかります。 また、これがマージされないことで、konashiがメンテされてないんじゃないかと見られるのも残念です。

非互換のAPIの変更は、副作用があるものなので、pullrequestの粒度を下げて、差分が見やすい形にしてauthorの判断を仰ぐ、というような形がよいのではないでしょうか。 自分であれば、かなり大規模に既存のAPIを壊すのはauthorが受け入れにくいのではないかと思うので、既存のものをそのまま残しつつ、 (別のpullreqで、非推奨であり、新しいAPIに移行していくことを既存ユーザーに示すようwarningを出すようにしてもいいかもしれません) 新しくKonashiPeripheralというようなクラスを導入して 複数台のKonashiを同時接続する機能の実装 をしていく、というふうに考えると思います。 非互換のAPIの変更を行うpullreqを出して、蹴られるのももったいないので...

まずは、この大きなpullrequestを、 非互換のAPIの変更、互換性を維持した機能の追加、blocksを使った便利クラスの導入、workspaceの導入などなど、意味をもったまとまりに分けて、pullrequestを分けてはいかがでしょうか。 このpullreqを書かれた方であれば、コミットをcherrypickしていくことでわりと大きな作業ではないかもしれません。

0x0c commented 10 years ago

@mash 様、コメントありがとうございます。 コミット内容が雑であったことを反省しております。 バージョンをメジャーアップすることで対応できるのではないかと、ユカイ工学の中のエンジニアの方と話したことからpull requestを投げたのですが、修正のため作業していたところ、マージがうまく行かず整理できませんでした。 また、整理していたところコンフリクトが発生し、また別のコミットが発生してしまいました。 pull requestを投げておきながら、誠に身勝手で申し訳ないのですが取り下げたいと思います。

失礼いたしました。

monakaz commented 10 years ago

0x0cさん、返事が遅くなって申し訳ありません。 まずはじめに、プルリクエストありがとうございます。 多岐に渡る変更がございますが、特に「メソッド名及び定数の命名規則の修正/統一」は影響が大きくすぐに反映することが困難であると考えております。

もともとのものをAurdinoの命名規則に合わせていた関係上、ObjectiveCの命名規則と合わなくなっているという指摘はすでにmashさんから頂いておりまして、変更の検討も行っていました。しかしやはりある程度ユーザがいる状況で、既存コードが動かなくなる変更は厳しいと判断しています。

mashさんがおっしゃられているように、Konashi.m, Konashi.hの上書きではなく、別クラスで定義し直す形でプルリクエストいただけると幸いです。こちらでもテストしてOKそうであれば、公式でも新しいAPI系統をアナウンスしてくこともできますので!(古い方はObsoluteにしつつ、そのまま置きつつ…)

引き続きよろしくお願いします!

0x0c commented 10 years ago

@monakaz 様、コメントありがとうございます。 下位互換を保持しつつ実現しつつ命名規則の変更やblocksの導入によるライブラリのアップデートは、既存のライブラリとマージする方法ではうまくいかないと考えております。 なぜなら、既存のライブラリの命名規則やAPIがObjective-CおよびCocoaとマッチしておらず、古いAPIを残したまま新しいAPIを実装するとクラスが肥大化してしまい、後々のメンテナンスに支障をきたす恐れがあります。 また、別クラスで定義するとなると同じ機能の持つクラスが複数存在することになり、後々APIを統一する際にライブラリのユーザが混乱するのではないかと思います。 なので、APIの更新は一度気に行うことが得策と考えています。 一度基に行う方法として、メジャーバージョンアップとして提供することが例として挙げられます。

ただ、今回のpull requestのコミットが良くないものであったことを反省しております。 改善してまたリクエストを遅らせていただきます。

今後共よろしくお願いいたします。