Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
整理: エンジンマニフェストをAPI向けと内部向けで分離 #1359
base: master
Are you sure you want to change the base?
整理: エンジンマニフェストをAPI向けと内部向けで分離 #1359
Changes from 13 commits
0bad672
cab1a75
e92ddee
68ec841
6821bb8
99e8812
84b04cf
36ba582
cd5fb22
32ab812
311e73c
656d4da
86ecb9d
ae970fa
ce135be
15bdf49
d67583c
daf249e
07f7cb7
71ab005
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この関数だけ見ると何をやっているのかかなりややこしいですね・・・
ManifestWrapperが何用なのか、wrapperからwrapperじゃないのを作るのがなぜgenerateなのか、だいぶ混乱しそうです。
ちょっと解決策は思いついてないのですが、どういう名付けをしていくのか考える良い機会な気がしました!
このファイルはAPI用なのか内部用なのかも混乱ポイントかもです。他のファイルに揃える形にすると良さそうかも(揃ってるかもですが。。状況がわかっておらず。。)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍️
妥当な指摘です。
まあ、wrapperとかadapterとかでも良さそう。
とのレビューコメントに基づきこの命名にしましたが、たしかにパッと見、よくわからないですね。このあたり、本質的にややこしいのが厄介です。以下の3種類のクラスが必ず必要になります:
EngineManifestJson
)ManifestWrapper
)EngineManifest
)どれもマニフェストの実体に関連するクラスなので、さてどう命名したもんか、という感じです。
B→C は cast ではあるけどファイル読み込みもしてるし、
generate_api_manifest()
は別物かと思っちゃう
のと指摘を頂いているし…。良い案があれば提案いただきたいです🙇
👍️
同意です。
ref #1385 (comment)
👍️
妥当な指摘です。
本 PR merge 後に router へ移動する PR を出す予定です。これにより API / router 用クラス(
EngineManifest
)と内部用クラス(ManifestWrapper
)がモジュール単位で分離します。There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すみません、これが内部用のデータ構造だと思ってなくてそうコメントしてました。。
良い名付けは思いつかないです。。ちょっと考えるの任せたいです…😇
良い前例に倣うのが手っ取り早い気はしています。どこに前例があるのか知らないのですが…。
とりあえず前例を調べまくってましたが、データ変換はDTOと呼ぶくらいしか出てきませんでした。
そもそもこのファイルは何の概念の中にあるのか、Api側は何の層の概念なのかとかを整理して名付けするとか…?
頭文字だけつける…?Model…?Inner…?
みたいな感じです…😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちなみに、そこまで実装したものをPRにするとどれくらいの変更量になりそうでしょうか。
プラマイ合計150くらいならそこそこコンパクトな気がしてます。
というのも、「途中なので一旦これで」となってるのは、実は最小機能よりも細かい変更をしていて、プルリクを小さく分けすぎてるかも?と思ったためです。
@tarepan さんにとって名付けに違和感少ないのであれば、もしかしたら完成系が浮かんでるからで、完成系ならこの名付け問題も起こってないかも、みたいな…。
ちょっと試しになのですが、この辺り完成まで一気に変更するとどうなるかPRお願いしても良いでしょうか……🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍️
承知しました。
命名はとても大事である一方、思い付かないときは思い付かないし最悪変な名前でも動く、との認識です。
author/reviewer 双方が名案を思い付かない場合、FIXME をつけつつ妥協案の名前を採用するのがベターだと考えます。良い変数名が思い付かないから機能追加/リファクタリングを merge しない、はプロジェクト全体の進行を考えるとアンバランスなケースが多そうです。
👍️
本 PR 内で移動含めてリファクタリングします。
以前のレビューで @Hiroshiba さんから「コード移動とコード変更を同時にしないでほしい」との旨の指摘を頂きました(無変更の純粋なコード移動であればレビューツールで自動検知できる的な話だったような)。
それ以降はこれに従い、サイズ関わらず移動と変更を別 PR に分割する方針を取っています。
「同時に移動/変更するのは OK」へ方針変更した、という認識で良いでしょうか?あるいは「今回は命名が難しいので全体像優先で同時に移動/変更」という感じでしょうか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
その時は僕がなんとかして考えます!
極端な話、関数名・変数名がぜんぶA B C D EみたいになってるPRは弾くと思います。
ちゃんと考えれてないですが、納得する言い方としては「機能が揃ってからmergeがよく、名前は機能の1つ」とかなのかなと。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどです!!
そういう意図だということに気づいていませんでした。
実装→移動 ではなく 移動→リファクタリング にするとどうでしょう・・・?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍️
困った場合はお任せします。
👍️
可能なケースではその順序で PR 提出する方針に変更します。