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

fix: IndexMap::{removeshift_remove} #846

Merged

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Oct 2, 2024

内容

indexmapを新しいバージョンに上げると以下の警告が出るため、その対応です。
(このPRではindexmapのバージョンはそのままにして、アップデートはRenovateに任せます)

warning: use of deprecated method `indexmap::IndexMap::<K, V, S>::remove`: `remove` disrupts the map order -- use `swap_remove` or `shift_remove` for explicit behavior.
   --> crates/voicevox_core/src/status.rs:274:19
    |
274 |         if self.0.remove(&model_id).is_none() {
    |                   ^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

warning: use of deprecated method `indexmap::IndexMap::<K, V, S>::remove`: `remove` disrupts the map order -- use `swap_remove` or `shift_remove` for explicit behavior.
  --> crates/voicevox_core/src/user_dict/dict.rs:64:56
   |
64 |         let Some(word) = self.with_words(|words| words.remove(&word_uuid)) else {
   |                                                        ^^^^^^

現在使っているindexmap v2.0.0の時点でremoveswap_removeshift_removeの三つは存在しており、removeswap_removeのエイリアスです。v2.1.0からremove#[deprecated]になって上記の警告が出るようになったようです。

swap_removeshift_removeの違いは何なのかと言うと、[a, b, c, d, e]のようなキーの並びからswap_removebを抜き取ると[a, e, c, d]のような並びになります。shift_removeはPythonのdict.__delitem__のような一般的な挙動です。

UserDict::remove_wordとかのパブリックAPIの挙動にも関わってくるため、一般的な挙動の方がよいかと思いswap_removeではなくshift_removeの方にしました。これが本PRのcommit prefixを"improve:"にしている理由です。
("fix:"にしようか迷ったのですが、ドキュメント上で順序の保証はしてなかったかなと思ったので"improve:"にしてしまいました
[追記] いや書いてた!!ということで、"fix:"にしました。

関連 Issue

#841

その他

新しいバージョンのindexmapで`IndexMap::remove`が`#[deprecated]`になるこ
とへの対応。ただしこのPRではindexmapのバージョンはそのままにして、アップ
デートはRenovateに任せる。

`#[deprecated]`対応とは別に`swap_remove`にしておく理由は特に無いため、
Pythonの`dict.{__delitem__,pop}`などに近い`shift_remove`にしておく。
@qryxip qryxip requested a review from Hiroshiba October 2, 2024 17:12
@qryxip qryxip changed the title improve: IndexMap::removeIndexMap::shift_remove improve: IndexMap::{removeshift_remove} Oct 2, 2024
@qryxip qryxip changed the title improve: IndexMap::{removeshift_remove} improve: IndexMap::{removeshift_remove} Oct 2, 2024
@qryxip qryxip changed the title improve: IndexMap::{removeshift_remove} fix: IndexMap::{removeshift_remove} Oct 3, 2024
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.

LGTM!!

順番入れ替わるの結構罠感ありますね!!
まあだからremoveが消える方針になったんだと思いますが。

@qryxip qryxip merged commit 6510a2a into VOICEVOX:main Oct 3, 2024
30 checks passed
@qryxip qryxip deleted the improve-indexmap-remove-to-shift-remove branch October 3, 2024 17:53
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