-
Notifications
You must be signed in to change notification settings - Fork 246
refactor: コアの読み込みをバージョン別に整理 #1645
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
Conversation
|
このPRの本題とは関係ないのですが、このPRで扱っているような「v0.12以降か否か」みたいなのと …という議論をVOICEVOX/voicevox_projectに移すという話があったのですが、そろそろやらないと…。 まあ少なくともこのリポジトリ内には現時点でそういう情報は無いので、一応の共有です。 |
| 直下にコア(共有ライブラリ)が存在するディレクトリ | ||
| use_gpu | ||
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.
#1620 (comment) にて「自明なのは省略しても良かったり」との指摘があったため、一旦そちらに寄せています。
これに関しても issue 化 + 議論を先行させたほうが良いでしょうか?
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.
あ、なるほどです!!
これはちょっと↓のコメントにも書いたんですけど、どちらも一長一短系なので、「そもそもなぜdocstringを書くのか」の方針を考えて、そこから導き出すと進めやすいのかなと思っています!!
これもなぜdocstringを書くのかなんて考えたことはないので、どうにかして言語化しておくとこういう時楽だよなぁとは思います。
とりあえずこのプルリクエストでの結論は、「省略可能なのか不可能なのかは置いといて、決まっていないのでここでは消しても別に良い」なのかなと思いました!!
決めておくと楽なので、もし決まってると便利そうであればissue作っていただければ・・・!!!
|
2点コメントしました! その箇所以外は LGTM です!! |
|
@Hiroshiba |
Hiroshiba
left a comment
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.
LGTM!!
内容
コアの読み込みをバージョン別に分けるリファクタリングを提案します。
コアの読み込みはコアバージョンの違いと fallback 処理によって複雑化している。他 PR のレビュー過程では、コアの読み込み周りがネストしているとの指摘があった。
これらを整理するには、第1段階としてコアの読み込みをバージョン別に分ける必要がある。
このような背景から、コアの読み込みをバージョン別に分けるリファクタリングを提案します。
また関連コードに lint 対象があり、かつ、diff が混線する心配が無いため、linting を同時におこなっています。
関連 Issue
無し