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

整理: root_dir デフォルト引数を指定 #1233

Closed
wants to merge 2 commits into from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented May 11, 2024

内容

概要: root_dir デフォルト引数を指定してリファクタリング

generate_app()root_dir 引数はデフォルトが None であり、None 時に engine_root() を代入している。
デフォルト引数を用いれば None チェックを削除してコードを簡略化できる。
engine_root() のデフォルト引数代入はリンターに弾かれるが、モジュールローカル定数として用意すれば問題ない。

このような背景から、root_dir デフォルト引数の指定によるリファクタリングを提案します。

関連 Issue

無し

@tarepan tarepan requested a review from a team as a code owner May 11, 2024 14:02
@tarepan tarepan requested review from Hiroshiba and removed request for a team May 11, 2024 14:02
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

違和感があってかなり迷ってたのですが、まあ良さそうかなぁ・・・。
一番いいのは関数の返り値をデフォルト値にするのを止めることかも・・・?

うーーーーーーん!!!難しい!!!迷ってます・・・!!


実行タイミングで初期値が変わりうるのがかなり危なそうに感じました。
中で感想を呼ぶ形だと必ず最新の関数が初期値として使われるのですが、この場合一番最初にモジュールをインポートしたタイミングで決定するんですよね・・・。
デフォルト値がimportタイミングで決定するのはそれなりに危ないなと。

この違和感を払拭するのはたぶん3通りやり方があるかも。
1つが今までの方法。
もう一つがengine_root()を定数にする方法。
最後がそもそもデフォルト値をやめる方法。

今回の場合はデフォルト値をやめるのも良いかもと思いました。
(最初に書いた通りだいぶ迷いました。。そんな超考えるべきところではないかもですが 😇 )

@tarepan
Copy link
Contributor Author

tarepan commented May 16, 2024

実行タイミングで初期値が変わりうるのがかなり危なそう

👍️
一般論として & エンジンの将来として同意です。
現在の engine_root() が実行タイミングに依存しないのを確認したうえで本 PR を作成しています。
ただ将来的に engine_root() 実装が変わったとき意図しないタイミング依存性を埋め込むリスクがあり、本 PR は依存性埋め込み時の危険性を跳ね上げます。

現状維持 or デフォルト値廃止の二択だと感じました。どちらがいいかは一長一短ですねぇ。前者は安全だが generate_app() が冗長、後者は安全だが caller 側が冗長で新しく呼び出すとき面倒。

本 PR は簡略化メリット < 将来的デメリットとなっているため close します。

@tarepan tarepan closed this May 16, 2024
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