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

整理: Setting をモジュール内に限局 #1397

Closed
wants to merge 5 commits into from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Jun 12, 2024

内容

概要: Setting をモジュール内に限局してリファクタリング

Setting はエンジンの設定(CORS 設定と origin 設定)をもつ BaseModel である。
router でも利用されており一見すると API モデルに見えるが、実際には内部向け構造体である。router ではコンテナ的に値を出し入れする役割を果たしているに過ぎず、BaseModel 由来の機能は YAML ファイル I/O 用のダンプ・バリデーションで使われている。
ゆえに API では CORS 設定と origin 設定を直接扱って問題ない。そして setting_manager モジュール内でのみ Setting 構造体を利用し影響範囲を適切に限局させるべきである。

このような背景から、Setting をモジュール内に限局するリファクタリングを提案します。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner June 12, 2024 05:52
@tarepan tarepan requested review from Hiroshiba and removed request for a team June 12, 2024 05:52
Comment on lines -46 to +48
return Setting(
cors_policy_mode=setting["cors_policy_mode"],
allow_origin=setting["allow_origin"],
)
setting_obj = _Setting.model_validate(setting)
return setting_obj.cors_policy_mode, setting_obj.allow_origin
Copy link
Member

@Hiroshiba Hiroshiba Jun 18, 2024

Choose a reason for hiding this comment

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

これはちょっと避けたいですね・・・。
どれがどれかわからず、フラグが2つ追加された時点でわからなくなり、またSetting構造体に戻ると思います。

内部型であってもrouterに露出するのは問題ないはずです。
例えばデータ変換のときとかは必ず渡さないとなので。
Settingをやり取りする形のがメンテナンス性高そうに思いました!

BaseModelじゃなくても良さそうには思います。

@tarepan
Copy link
Contributor Author

tarepan commented Jun 18, 2024

またSetting構造体に戻る ... Settingをやり取りする形のがメンテナンス性高そうに思いました

👍️
妥当な指摘です。

本 PR の懸念点である 一見すると API モデルに見える#1405 に基づく dataclass 化によって緩和できそうな見通しです。
よって本 PR はデメリットが勝ると判断し close とします。
なお、dataclass 化は別 PR にて着手する予定です。

@tarepan tarepan closed this Jun 18, 2024
@tarepan tarepan deleted the refactor/setting_simple branch June 18, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants