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

multistreamEnabled を非推奨扱いにする #225

Merged
merged 5 commits into from
Jan 27, 2025

Conversation

zztkm
Copy link
Contributor

@zztkm zztkm commented Jan 24, 2025

  • [UPDATE] multistreamEnabled を非推奨扱いにする
    • multistreamEnabled の設定が不要なイニシャライザを Configuration に追加する
    • ドキュメントコメントに非推奨扱いの旨を追加する

This pull request deprecates the multistreamEnabled setting in the Configuration struct and updates related documentation and initializers to reflect this change. The most important changes include marking multistreamEnabled as optional, updating initializers to handle the optional type, and modifying the logic to accommodate the deprecation.

Deprecation of multistreamEnabled:

  • CHANGES.md: Added an entry to mark multistreamEnabled as deprecated and updated documentation comments accordingly.
  • Sora/Configuration.swift: Updated the multistreamEnabled property to be optional and modified related documentation comments to indicate its deprecation. [1] [2]

Initializer updates:

Logic adjustments:

  • Sora/PeerChannel.swift: Updated the logic to handle the optional multistreamEnabled property and ensure compatibility with the spotlightEnabled setting.

@zztkm zztkm self-assigned this Jan 24, 2025
@zztkm zztkm changed the title [WIP] multistreamEnabled を非推奨扱いにする multistreamEnabled を非推奨扱いにする Jan 27, 2025
@zztkm zztkm requested a review from torikizi January 27, 2025 08:39
Copy link
Contributor

@torikizi torikizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

タイポと、一点だけ教えてほしい事項があります。

Sora/Configuration.swift Outdated Show resolved Hide resolved
var multistream = configuration.multistreamEnabled
if configuration.spotlightEnabled == .enabled {
multistream = true
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっとわからなくなってしまったので教えて欲しいのですが、ここはなぜ変更したのでしょうか

Copy link
Contributor Author

@zztkm zztkm Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

元のコードだと configuration.multistreamEnabled が Bool? の場合に、configuration.multistreamEnabled || configuration.spotlightEnabled == .enabled のままだと configuration.multistreamEnabled を !! したり ?? でデフォルトを与えたりして Bool 型にする必要がありました(そうしないとコンパイルエラーになります)。

今回の変更の目的は multistreamEnabled は Bool? として nil (指定なし) を導入することにあるので、これだと目的が達成されません。

なので、297 行目で一度 var multistream: Bool? を定義して、そのあと spotlightEnabled であれば true に上書きするというコードに変更しました。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ありがとうございます。理解できました。
同じような処理なのでなぜかなと変なところにはまり込んでしまってました。

Copy link
Contributor

@torikizi torikizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

修正ありがとうございました。 LGTM です。

@zztkm zztkm merged commit e27eca4 into develop Jan 27, 2025
4 checks passed
@zztkm zztkm deleted the feature/deprecated-multistream-option branch January 27, 2025 10:07
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