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

Remove: OjtPhoneme 特殊Equal廃止 #787

Closed
3 tasks done
tarepan opened this issue Nov 27, 2023 · 2 comments · Fixed by #800
Closed
3 tasks done

Remove: OjtPhoneme 特殊Equal廃止 #787

tarepan opened this issue Nov 27, 2023 · 2 comments · Fixed by #800

Comments

@tarepan
Copy link
Contributor

tarepan commented Nov 27, 2023

内容

要望: OjtPhoneme.__eq__() の廃止

現在の OjtPhoneme は built-in関数 __eq__() をオーバーライドし特殊なEqualチェックをおこなっている。
__eq__() 内では .start/.end 属性に基づく判別をおこなっているが、この属性は現在使われていない(placeholder値が代入されるだけ)。また voicevox_engine 内でもこの特殊Equalを生かした操作は現在実装されていない。

一方で特殊Equalは検索が不可能である。メソッドと異なり、OjtPhoneme のインスタンスがEqualチェックされている箇所を一発で検索することはできない(インスタンス名の自由度、 ==in など多数の演算子)。
結果として OjtPhoneme 変更の影響範囲把握が著しく困難になり、リファクタリング含む変更を阻害している。

上記の理由より、OjtPhoneme.__eq__() の廃止を提案します。

(本issue/PRは影響しうる範囲が広く変更リスクが高いため、OjtPhoneme.__eq__() 廃止に焦点を絞り、廃止後のリファクタリングは別に行う予定です。)

Pros 良くなる点

  • 複雑性の低減
  • 変更容易性の向上

Cons 悪くなる点

  • (未使用の)機能性低下

実現方法

  • OjtPhoneme.__eq__() の削除

VOICEVOXのバージョン

0.14.10

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

廃止の影響範囲確認が非常に煩雑かつ重要であるため、pull request時のコメントにて影響範囲一覧(と機能的影響がない証明)を付記する予定です。

@github-actions github-actions bot added OS 依存:linux Linux に依存した現象 OS 依存:mac macOS に依存した現象 OS 依存:win Windows に依存した現象 labels Nov 27, 2023
@Hiroshiba
Copy link
Member

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

演算子のオーバーロードは確かに検索性が良くないので、結構な恩恵が受けられる時のみ実装するのが良いのかなと感じました!
明確な比較が必要な時は別途関数を定義したり、その都度プロパティの比較をした方が分かりやすいのかなと。
(この辺に関してベストプラクティスを探してみたのですが、特に資料は見つかりませんでした)

なので__eq__を廃止する方針が良いのかなと思いました!!

@tarepan
Copy link
Contributor Author

tarepan commented Nov 28, 2023

👍
#788 に依存しているため、当該PRマージ後に着手します。
着手しました。

@tarepan tarepan removed OS 依存:mac macOS に依存した現象 OS 依存:linux Linux に依存した現象 OS 依存:win Windows に依存した現象 labels Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants