VOICEVOX / open_jtalk-rs

BSD 3-Clause "New" or "Revised" License
10 stars 12 forks source link

`impl Iterator` → `impl IntoIterator` #11

Closed qryxip closed 1 year ago

qryxip commented 1 year ago

タイミングが今で申し訳無いのですが、 #9 で入ったコードに対する提案です。

こちらの方がidiomaticかと思います。

PickledChair commented 1 year ago

なるほどです! 言われてみれば、なんとなくこちらの型指定の方が多いかもですね。

後学のためにお聞きしたいのですが、この型指定にすることで得られるメリットが明らかにある場合教えてくださると嬉しいです。また、このような「こう書いた方がより良い」という指針はどこから得られましたか? なんとなく、確かに標準ライブラリはこんな感じの引数にしてあるなと思い当たったのですが……。

qryxip commented 1 year ago

この型指定にすることで得られるメリットが明らかにある場合

利点としてはVec<T>&'a [T]のような型の値を直接引数に入れられることでしょうか? ただimpl Iteratorが良いというよりもimpl Iteratorだと中途半端ではないかと思ったのがPRの動機です。私が1から書いたら&[impl AsRef<Path>]とか&[PathBuf]とかにしていたかもしれません。

12 もですが、 #9 のレビュー中だったらsuggestion投げていたかなというくらいの温度感でいます。

また、このような「こう書いた方がより良い」という指針はどこから得られましたか?

あえて出典を挙げるならRust API GuidelinesのC-GENERICでしょうか? まあこれはpublicなAPIについての話ですが。

PickledChair commented 1 year ago

なるほどです、ありがとうございます。

私が1から書いたら&[impl AsRef<Path>]とか&[PathBuf]とかにしていたかもしれません。

これは確かに私もレビューしているときに同様のことを感じました。しかし「変えなければいけない」というほどの理由が自分の中にはなかったのでそのままにした感じです。役割を十分果たしていればそれで問題ない、と判断しました。この関数は他の場所で再利用される場面はほぼないと考えて良さそうだったのと、仮にあったとしてもそのときに適切な形に変えれば良い、という判断もありました。

(正直にいうと「コミュニケーションの数を最適化したい」という気持ちもありました。個人的に最近忙しくなってきているので、時間的・体力的なリソースを潤沢に振り向けられなくなってきています。ここはちょっと申し訳なさがあります。明らかに重大な議題であったならそれでも十分議論を尽くすつもりでいますが……)

利点としてはVec<T>&'a [T]のような型の値を直接引数に入れられること

という意見は確かにそうだな、と感じ勉強になりました。参考リンクもありがとうございます。ただ上に書いた観点からは、個人的には「変更前でも変更後でもどちらでも良い」という印象でした。

(誤解を避けるため念のためお伝えしますが、今回のように「この方がより良いかも・より自然かも」という提案や視点をいただけること自体は非常にありがたいと思いました。)

qryxip commented 1 year ago

これは確かに私もレビューしているときに同様のことを感じました。しかし「変えなければいけない」というほどの理由が自分の中にはなかったのでそのままにした感じです。役割を十分果たしていればそれで問題ない、と判断しました。この関数は他の場所で再利用される場面はほぼないと考えて良さそうだったのと、仮にあったとしてもそのときに適切な形に変えれば良い、という判断もありました。

なるほど。レビュー判断としてそのままにしたんですね。

(正直にいうと「コミュニケーションの数を最適化したい」という気持ちもありました。個人的に最近忙しくなってきているので、時間的・体力的なリソースを潤沢に振り向けられなくなってきています。ここはちょっと申し訳なさがあります。明らかに重大な議題であったならそれでも十分議論を尽くすつもりでいますが……)

そこはお時間取ってしまって申し分けなかったなーと思ってます。

実はこのPRは #12 と合体して"Fix up #9"のようなタイトルで出そうと思ってたのですが、qwertyさんが書いていた部分で一箇所だけ.display()が使われている所があったためPRを分離したんですよね。

変えるべき理由は特に無い...けど変えない理由も特に無いし、レビュー中ならsuggestion出してた。 #9 マージ直後の今ならまだセーフなのでは?という思考でこのPRを出しました。こう、なんというか3秒ルール的な感じで (本物の3秒ルールは衛生的に駄目ですが)。

PickledChair commented 1 year ago

あー、なるほどです。本来は先の PR と同時に改善しようとしていた、という事情がわかりました。なんにせよ、提案自体はなるほどなと思ったのでありがとうございます!