Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SwiftLint 導入を含めたリファクタ対応 #6

Merged
merged 4 commits into from
Feb 22, 2017

Conversation

zdogma
Copy link
Owner

@zdogma zdogma commented Feb 21, 2017

背景

下記の Issue に対応した。
fixed #3 SwiftLint を導入したい
fixed #4 SwiftLint に則って Caution 対応したい
fixed #5 Song のデータの型(Entity)を定義して持ち回したい

TODO リスト

ほかに気になっていること

  • テストどうするんだっけ
  • 残された force_unwrapping の対処法
    • 自分で設計したオプショナルなら回避できるはず、組み込み API が返すような挽回不可な nil は意図的にクラッシュさせるのであれば OK
  • swiftlint.yml のおすすめ
    • まずは自分のコーディング規約の思想に基づく範囲でフル適用、その後妥協していく
  • アプリにおける例外のレスキューのお作法
    • (webも同じだが)握り潰さないこと
      • ハンドリングの思想に関しては こちら

備考

  • ユーザーへの警告のために SCLAlertView を利用

Copy link
Owner Author

@zdogma zdogma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

パッと見で意図がわかりづらそうな部分に関して補足

);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "if which ${PODS_ROOT}/SwiftLint/swiftlint >/dev/null; then\n ${PODS_ROOT}/SwiftLint/swiftlint\nelse\n echo \"warning: SwiftLint not installed, download from https://github.com/realm/SwiftLint\"\nfi";
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SwiftLint 追加対応に関する変更 🍸

pod 'SCLAlertView'
end

post_install do |installer|
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

下記への対応 🍸

“Use Legacy Swift Language Version” (SWIFT_VERSION) is required to be configured correctly for targets which use Swift. Use the [Edit > Convert > To Current Swift Syntax…] menu to choose a Swift version or use the Build Settings editor to configure the build setting directly.

http://qiita.com/yokomotod/items/a96ab24f01a91d54d399

@keitaoouchi
Copy link
Collaborator

うん、このPRはLGTMっす。

force_unwrapping

force_unwrappingは自分で設定したOptionalに対して使う場合は型の設計がおかしいと思われるのでNG、UIKitとかFoundationのAPIがOptionalを返してきて、仮にnilだったら挽回不能なので敢えてクラッシュさせてしまう(例外を投げさせてしまう)という意図的な使い方はOKって感じです。

swiftlint

プロジェクトはじめるときは基本的に全部のルールをONにして徐々に妥協してく感じですかねー。

例外の作法

なんつっても例外を握りつぶさないってとこですね。
こちらも参照してみてください。
https://ez-net.jp/article/B2/cwtBwURB/pp3ZiIOQz2Cc/
Result型でのError表現などにも触れられています。

Result型使いたいときは
https://github.com/antitypical/Result
これが良いです。

@zdogma
Copy link
Owner Author

zdogma commented Feb 22, 2017

@keitaoouchi
なるほどです、ありがとうございます!
下記2点はこのアプリ開発の中で経験として積んでおきたいこともあり、別途検討の上で PR にしたいと思いました!

  • force_unwrapping
  • エラーハンドリング

一旦この PR はこちらでマージします!
レビューありがとうございました!
マージ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants