Skip to content

Conversation

@iiweis
Copy link
Contributor

@iiweis iiweis commented Nov 29, 2025

I thought some parts of streaminghub/getting-started.md could be incorrect, so I made some adjustments.

@iiweis iiweis requested a review from mayuki as a code owner November 29, 2025 15:48
Comment on lines +154 to 155
room.All.OnLeave(userName);
await room.RemoveAsync(Context);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought RemoveAsync should be called first here, but I left it as is.
By the way, in the implementation of samples/ChatApp, RemoveAsync is called first.

await this.room.RemoveAsync(this.Context);
this.room.All.OnLeave(this.myName);

@mayuki
Copy link
Member

mayuki commented Dec 1, 2025

Thank you for the PR!
Since the receiver events is handler-driven, perhaps names like the following would have been better:

  • OnJoined
  • OnLeft
  • OnMessageReceived

@iiweis
Copy link
Contributor Author

iiweis commented Dec 1, 2025

Thank you for the confirmation.

I agree that the suggested name is more intuitive and easier to understand.

However, if we change the name, I believe the sample implementation should also be revised accordingly.
Since this PR is intended for "fixing non-functional parts" and "ensuring consistency in descriptions" within the documentation,
I think it might be better to either merge this PR first and then make the name change, or address it all together in a separate PR.

What are your thoughts on this approach?

@mayuki
Copy link
Member

mayuki commented Dec 2, 2025

Ah, I see. I hadn't considered this article and ChatApp to be the same thing.
It seems best to just fix the parts that aren't working here.

@iiweis
Copy link
Contributor Author

iiweis commented Dec 3, 2025

I hadn't considered this article and ChatApp to be the same thing.

I see.
In that case, I'll limit the fixes to the parts that aren't working right now.

That said, I personally believe there’s no downside to keeping interface definitions consistent.
So, along with the method name changes, I’d appreciate it if you could also consider future modifications.

@iiweis iiweis force-pushed the update-streaminghub-docs branch from 3a68b32 to 11e5d9a Compare December 3, 2025 23:59
@iiweis
Copy link
Contributor Author

iiweis commented Dec 4, 2025

I’ve made the fix.
ToString was written in camelCase and wasn’t working, so I updated it to use userName as an arg instead, which I believe is more appropriate.

@mayuki
Copy link
Member

mayuki commented Dec 4, 2025

Thanks!

@mayuki mayuki merged commit 236ecd0 into Cysharp:main Dec 4, 2025
1 check passed
Copilot AI added a commit that referenced this pull request Dec 4, 2025
mayuki added a commit that referenced this pull request Dec 4, 2025
…docs

Sync PR #990 fix to Japanese and Korean documentation
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