refactor: 引数の数を減らす

[chun37]

voicevox_engine/voicevox_engine/app/application.py

Lines 41 to 55 in 18ce0a2

	def generate_app(
	tts_engines: TTSEngineManager,
	song_engines: SongEngineManager,
	core_manager: CoreManager,
	setting_loader: SettingHandler,
	preset_manager: PresetManager,
	user_dict: UserDictionary,
	engine_manifest: EngineManifest,
	library_manager: LibraryManager,
	cancellable_engine: CancellableEngine | None = None,
	character_info_dir: Path | None = None,
	cors_policy_mode: CorsPolicyMode = CorsPolicyMode.localapps,
	allow_origin: list[str] | None = None,
	disable_mutable_api: bool = False,
	) -> FastAPI:

ここで渡している引数は Engine の Config だと読み取れる。
EngineConfig や Config のような名前の dataclass を用意して渡したほうがスマートだと思いました。

[tarepan]

提案ありがとうございます！

EngineConfig や Config のような名前の dataclass を用意して渡し

こちらの具体的な内容が知りたいです。

@dataclass
class Config:
  tts_engines: TTSEngineManager,
  ...

という感じだという理解で正しいでしょうか？

その他コメント

リファクタリング issue 作成時は、issue テンプレート「Feature Request（機能要望・改善提案）」の利用が推奨です。

[chun37]

run.py の役割としては

• 環境変数を取得する
• コマンドライン引数を取得する
• 環境変数やコマンドライン引数の優先度付けを行う
• 指定された値が不正でないか確認する
  くらいにしたく、manager や engine をインスタンス化するのはやり過ぎだと思っています。

1. run.py でパラメータをまとめる
2. Config というクラスにまとめる
3. generate_app に Config を渡す
4. アプリケーションに必要な manager や engine などをインスタンス化する

という形にするのが、単一責任の原則に従っている状態であると考えています。

引数が多いと、渡し忘れが発生したり関数の変更コストが上がるので、あまり良くないと考えています。

[tarepan]

manager や engine をインスタンス化するのはやり過ぎ ... 単一責任の原則

👍️
「app 的なエンティティがある」という立場を取った場合、妥当な指摘だと感じます。

別の理解の仕方として、「各機能を担う独立したエンティティ」「機能間連携を担う app」「WebAPI を提供するサーバー」という観点からみたとき、現行の分割もまた妥当だと考えています。
この観点でみると、例えばテストに関して「一部の機能はテスト用に切り替えたいので、テスト用にエンティティを作り、appに渡す」という簡潔な理解ができます。
「app 的なエンティティがある」という観点の場合、テストに関して「app内にテスト用の切り替えコードを仕込み、config 経由でテスト用に振る舞いを切り替える」という理解になります。これはむしろ、関心の分離（単一責任原則）と逆行していると感じます。app の主たる役割は機能間連携なのに、それと無関係なテスト用切り替えが混入しているからです。
ラフな言い方をすれば「DI による単一責任原則の遵守を重視しているのでいまのアーキテクチャになっている」と言えそうです。

引数が多いと、渡し忘れが発生したり関数の変更コストが上がる

Config でも同様の議論（例: 「Config の属性が多いと、渡し忘れが発生したり Config の変更コストが上がる」）になると考えます。
本質的にはコードの複雑性が原因であり、多様な機能からくる避けられない複雑性なら型検査で抜けを防ぐのが現実的だと思います。

[chun37]

これはむしろ、関心の分離（単一責任原則）と逆行していると感じます。app の主たる役割は機能間連携なのに、それと無関係なテスト用切り替えが混入しているからです。

確かにそうですね。
かといって、テスト用の app を作成するのは保守コストが上がりますし、本当にそれでテスト出来てるのかみたいな問題もありますし、今のアーキテクチャで問題ないと言えます。

Config でも同様の議論（例: 「Config の属性が多いと、渡し忘れが発生したり Config の変更コストが上がる」）になると考えます。

Config を分けるなどして、わかりやすくする方法を取ることもできると思います。
「Webサーバ設定」「音声設定」みたいな粒度で小さくできるのではないでしょうか？

型検査で抜けを防ぐのが現実的だと思います。

ここに関しては同意です。