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

blocking版とtokio版のimplmod内に移す #710

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Dec 18, 2023

内容

#702 で実装したblocking版のimpltokio版のimplは、Gitのdiffを出さないためだけにこうしていました。

impl self::blocking::PublicStruct {
    // …
}

impl self::tokio::PublicStruct {
    // …
}

pub(crate) mod blocking {
    pub struct PublicStruct;
}

pub(crate) mod tokio {
    pub struct PublicStruct;
}

これを次のようにします。

pub(crate) mod blocking {
    pub struct PublicStruct;

    impl self::PublicStruct {
        // …
    }
}

pub(crate) mod tokio {
    pub struct PublicStruct;

    impl self::PublicStruct {
        // …
    }
}

関連 Issue

その他

@qryxip qryxip requested a review from Hiroshiba December 18, 2023 16:50
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!!

pub(crate) mod blocking内でimportするのか.rsファイルの一番上でimportするのかで揺れてしまいそうかもと思いました!
blockingだけビルドしているときにasync用のライブラリが不要なようにするという意図だと思うので、基本はファイルの一番上でimportしてasyncで必要なものだけmod async内でipmortする、というのもありかも?
(ただのアイデアです)

@Hiroshiba Hiroshiba merged commit e115285 into VOICEVOX:main Dec 19, 2023
31 checks passed
@qryxip
Copy link
Member Author

qryxip commented Dec 23, 2023

まずmodでのuseですが、次の2つはほぼ同じ意味です。

// ./blocking.rs内に内容を書く
pub(crate) mod blocking;
pub(crate) mod blocking {
    // 内容
}

本来blocking.rsやtokio.rsと分けるところを、見やすさのために同じファイルに含めている状態です。

そしてどちらにおいてもuseは子モジュールに影響しません。親モジュールのuseを継承するには

use super::*;

とする必要があります。

今回は #708 の方針を継続して、すべて明示的なインポートとしています。

@Hiroshiba
Copy link
Member

あ、そうなんですね!!! Pythonの気持ちで考えていました。
以前は親モジュールに直接implしていたから親のuseを使えていたけど、子モジュールのimplになったから子モジュールでuseする必要が出てきた、という理解です。
ありがとうございます!!

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