既存コードをAPI Guidelineに準拠させる #72

daikiumehara · 2024-04-30T02:27:33Z

変更点

API Guidelineに準拠

Areaのenumをupper camel caseからlower camel casaeに変更
callback〇〇のメソッドからcallbackの文言を削除

リファクタ

エラーのキャスト処理を修正
makeResponseメソッドを統一

daikiumehara

個人的に気になった箇所を修正してみました
commit分けてあり、簡単に変更取り消せるので、変更がおかしければバシバシ言っていただけると🙏

daikiumehara · 2024-04-30T02:34:06Z

Sources/YumemiWeather/YumemiWeather.swift

@@ -183,14 +185,14 @@ final public class YumemiWeather {
    /// - Parameters:
    ///   - jsonString: 地域と日付を含む JSON 文字列
    ///   - completion: 完了コールバック
-    public static func callbackFetchWeather(_ jsonString: String, completion: @escaping (Result<String, YumemiWeatherError>) -> Void) {
+    public static func fetchWeather(_ jsonString: String, completion: @escaping (Result<String, YumemiWeatherError>) -> Void) {


引数にcompletionが用意されているため、callbackという文字は意味が重なっているように感じたので削除しました👀

もしかすると元々はコールバック版であることを分かりやする狙いがあったのかもしれないですけれど、こちらの方が自然で良い気がします。

⋯と思ったのですけれど、周囲を見渡してみると現状では次のようにバランスが崩れる印象でした。

同期版 — syncFetchWeather

非同期版 — asyncFetchWeather

コールバック版 — fetchWeather（旧 callbackFetchWeather）

名前的には callback が付かない方が綺麗に感じるのですけれど、そうなると同期版と非同期版もそのように揃えたくても async メソッドと通常メソッドでは同じ関数シグネチャを持てない問題があり、実現できないところです。

とはいえ callbackFetchWeather に戻すのは微妙な気がします。どのようにするのが良いでしょうね。

こちら、アイデアの方向性は良いものの方針を定めにくいので、今回はいったん名称を callbackFetchWeather に戻して、改めて別の議題に切り出して考えると良いアイデアが見つかるかもしれないです。

Issue として #77 に登録して、この部分はいったん元に戻してみますね。

daikiumehara · 2024-04-30T02:34:33Z

Sources/YumemiWeather/YumemiWeather.swift

            }
-            catch let error where error is YumemiWeatherError {
-                completion(Result.failure(error as! YumemiWeatherError))
+            catch let error as YumemiWeatherError {


エラー処理のキャスト処理をコンパクトにしました

上手な型キャストパターンの扱い方です。

Sources/YumemiWeather/YumemiWeatherList.swift

daikiumehara · 2024-04-30T06:16:13Z

これPackageしか編集してないからサンプルコード動くように修正しないといけない💦

daikiumehara · 2024-04-30T07:29:58Z

もっと小さいスコープで見ることが重要そうなので、
色々修正入るかもです🙇‍♂️
（細かくPR出した方が良さそうなので、別でPR作るかもです）

es-kumagai · 2024-04-30T14:11:09Z

そもそもの Example のテストコードに不備があったので修正しますね。

yumemi-kumagai · 2024-04-30T15:02:33Z

ここまでで、現状のコードでの Example プロダクトのテストがひとまず通るようになりました。

yumemi-kumagai · 2024-04-30T15:46:12Z

Sources/YumemiWeather/YumemiWeather.swift

-            catch let error where error is YumemiWeatherError {
-                completion(Result.failure(error as! YumemiWeatherError))
+            catch let error as YumemiWeatherError {
+                completion(.failure(error))
            }
            catch {
                fatalError()


せっかくなので、通常は有り得ないとはいえ無言で fatalError するのは控えておくと良いかもしれません。

Suggested change

fatalError()

fatalError("An unexpected error has occurred: \(error.localizedDescription)")

…を修正しました。

現状の WeatherViewController には反映完了を検出する術がなさそうだったため、やや強引かつ不確実なテストコードになりました。

…方法にしました。

…不必要に IUO オプショナルを使わないようにしました。

省略推奨についての確かな根拠は見つけられませんでした。以前に The Swift Programming Language で「通常は省略する」という記載があった気がしたのですけれど、探しても見つけられなかったため、省略しない方針もアリかもしれません。その場合はこのコミットを Revert しましょう。

This reverts commit 8ba2766.

es-kumagai

難度の高い次の２項目は別の PR に切り出すことで、この PR はいったん、既存コードの細かな調整のひとつとして収めて良さそうな印象を持ちました。

Areaのenumをupper camel caseからlower camel casaeに変更
callback〇〇のメソッドからcallbackの文言を削除

es-kumagai · 2024-05-01T14:15:54Z

もっと小さいスコープで見ることが重要そうなので、色々修正入るかもです🙇‍♂️ （細かくPR出した方が良さそうなので、別でPR作るかもです）

こちら賛成で、他の気になる箇所については粒度を小さめに PR を出していくと良いかもしれないですね。

…沿った方法に変更しました。意図した乱数を作り出すために makeRandomResponse に seed を渡す設計になっていましたが、現在の標準ライブラリーでは some RandomNumberGenerator を渡すのが一般的のため、それに合わせました。また、seed によって乱数が制御されることは ControllableGenerator 器固有の仕様なため、makeRandomResponse には渡さず、それを ControllableGenerator 自身が制御するようにしました。この際、内部で使われる srand48 関数はグローバルに影響するため、イニシャライザーで呼び出すとほかのインスタンスにも影響します。その性質を考慮して、ControllableGenerator はシングルトンで実装するようにしました。加えて、seed のリセットも ControllableGenerator の範疇と思われるため、必要な値を受け取って適切にシードをリセットする機能もここに移動しました。その他として、シングルトンを static var で保持させています。これは乱数生成器を各種 API が inout で受け取る仕様になっているため、そこにそのまま渡せるようにするための措置です。自由に書き換え可能にはなりますが、そもそもインスタンスが１つしか存在せず、そのイニシャライザーはプライベートで保護されているため、予期しないインスタンスの入れ替え操作は起こりません。

daikiumehara · 2024-05-08T03:54:53Z

Tests/YumemiWeatherTests/YumemiWeatherListTests.swift

+
+        let response = try YumemiWeather.decoder.decode([AreaResponse].self, from: responseData)
+
+        XCTAssertEqual(response.map(\.area), Area.allCases)


こちら空の場合にすべて取得されるのが正しい挙動ですかね？
であればDocument commetに追記しておいた方が良さそうと思いました！

たしかに⋯！

地域指定が空の場合にすべての地域が得られることが正しい意図かはわからないものの、課題で使える可能性があるのは TableView の課題で、それが意図されていることが a2fe1a8 から予想できそうです。

TableView の課題新設と併せて List バージョンの API が用意されたのと、そのときに合わせて実装されたテストコードでも空だったときに全てを返すテストが用意されているのを踏まえると、妥当と思って良さそうな気がします。

そうするとたしかに、ドキュメントコメントに明記しておくと良さそうですね。

6d986e1 で対応しました。

ちゃんとコード読めてなかったです🙏
対応ありがとうございます！

…ました。擬似 API であることの記述を各 API ではなく YumemiWeather 型のドキュメントコメントに記載しました。それに伴い、単体取得用の API でのドキュメントコメントも調整し、全体に統一感を持たせました。

YumemiWeather API が無作為にエラーを返す頻度をコントロール可能にしました。

edit: rename callbackFetchWeatherList to fetchWeather

8ba2766

daikiumehara changed the title ~~Feature/apply api guideline~~ API Guidelineに準拠させる Apr 30, 2024

daikiumehara changed the title ~~API Guidelineに準拠させる~~ 既存コードをAPI Guidelineに準拠させる Apr 30, 2024

daikiumehara self-assigned this Apr 30, 2024

daikiumehara commented Apr 30, 2024

View reviewed changes

ykws requested a review from a team April 30, 2024 06:11

yumemi-kumagai reviewed Apr 30, 2024

View reviewed changes

daikiumehara and others added 11 commits May 1, 2024 11:26

edit: remove unnecessary makeResponse code

9520e85

edit: refactor casting process

d385448

Example のテストコードで WeatherModel プロトコルの要求を満たせていなかったのを修正しました。

ebd0419

Example テストコード内で、loadWeather メソッドにその呼び出し主が渡されていないのを修正しました。

50276c9

WeatherViewController に DisasterModelMock が設定されていないことが原因で、テストが強制終了するの…

70f228f

…を修正しました。

テストコード内の typo を修正しました。

f8d13ae

テストコードでの View の準備は、ビューへのプロパティーアクセスを捨てる方法ではなく 'loadViewIfNeeded` を呼び出す…

81f35eb

…方法にしました。

少なくとも現状のテストコードでは WeatherModelMock を 'setUpWithError' まで待たなくても準備できるため、…

c1fb6be

…不必要に IUO オプショナルを使わないようにしました。

単独で存在していた、不必要なドキュメントコメントを削除しました。

acb3dda

daikiumehara force-pushed the feature/apply_api_guideline branch from 8365b54 to ee6cf6d Compare May 1, 2024 02:26

yumemi-kumagai mentioned this pull request May 1, 2024

YumemiWeather の公開 API を自然な名前づけにしたい（既存コードをAPI Guidelineに準拠させる） #77

Open

es-kumagai added 3 commits May 1, 2024 21:54

ThreadBlock の課題文に、使用する API の特徴説明を追記しました。

17daa5f

YumemiWeather クラスの見通しを良くするために、外部公開する API の機能とそれ以外とを区別して定義するようにしました。

b7bf09c

Revert "edit: rename callbackFetchWeatherList to fetchWeather"

4a081da

This reverts commit 8ba2766.

es-kumagai approved these changes May 1, 2024

View reviewed changes

不必要な名前空間の指定を削除しました。

6965ed4

es-kumagai added 6 commits May 2, 2024 15:55

エンコードされたものが適切にデコードできるように修正しました。

ddef528

標準ライブラリーの作法に基づいて、WeatherCondition をランダムで生成するコードを変更しました。

121b753

不必要な名前空間指定を削除しました。

3193e27

ドキュメントコメントの誤字を修正しました。

718cc61

不必要な名前空間の明記を省略しました。

feee440

daikiumehara commented May 8, 2024

View reviewed changes

yumemi-kumagai and others added 3 commits May 9, 2024 22:22

YumemiWeather API が無作為にエラーを返す頻度をコントロール可能にしました。

9cfb85b

Merge pull request #81 from yumemi-inc/feature/controllable_api_error

95aa9c9

YumemiWeather API が無作為にエラーを返す頻度をコントロール可能にしました。

es-kumagai merged commit 55bdcfb into main May 23, 2024
2 checks passed

es-kumagai deleted the feature/apply_api_guideline branch May 23, 2024 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

既存コードをAPI Guidelineに準拠させる #72

既存コードをAPI Guidelineに準拠させる #72

daikiumehara commented Apr 30, 2024 •

edited

Loading

daikiumehara left a comment

daikiumehara Apr 30, 2024

yumemi-kumagai Apr 30, 2024

yumemi-kumagai Apr 30, 2024

yumemi-kumagai May 1, 2024

daikiumehara Apr 30, 2024

yumemi-kumagai Apr 30, 2024

daikiumehara commented Apr 30, 2024

daikiumehara commented Apr 30, 2024 •

edited

Loading

es-kumagai commented Apr 30, 2024

yumemi-kumagai commented Apr 30, 2024

yumemi-kumagai Apr 30, 2024 •

edited

Loading

es-kumagai left a comment

es-kumagai commented May 1, 2024

daikiumehara May 8, 2024

yumemi-kumagai May 9, 2024 •

edited

Loading

yumemi-kumagai May 9, 2024

daikiumehara May 14, 2024

	fatalError()
	fatalError("An unexpected error has occurred: \(error.localizedDescription)")


		let response = try YumemiWeather.decoder.decode([AreaResponse].self, from: responseData)

		XCTAssertEqual(response.map(\.area), Area.allCases)

既存コードをAPI Guidelineに準拠させる #72

既存コードをAPI Guidelineに準拠させる #72

Conversation

daikiumehara commented Apr 30, 2024 • edited Loading

変更点

API Guidelineに準拠

リファクタ

daikiumehara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daikiumehara commented Apr 30, 2024

daikiumehara commented Apr 30, 2024 • edited Loading

es-kumagai commented Apr 30, 2024

yumemi-kumagai commented Apr 30, 2024

yumemi-kumagai Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

es-kumagai left a comment

Choose a reason for hiding this comment

es-kumagai commented May 1, 2024

Choose a reason for hiding this comment

yumemi-kumagai May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daikiumehara commented Apr 30, 2024 •

edited

Loading

daikiumehara commented Apr 30, 2024 •

edited

Loading

yumemi-kumagai Apr 30, 2024 •

edited

Loading

yumemi-kumagai May 9, 2024 •

edited

Loading