Skip to content

api: Rename StreamMessage to ChannelMessage #848

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

Closed

Conversation

Khader-1
Copy link
Collaborator

Fixes parts of #631

@Khader-1 Khader-1 requested a review from sm-sayedi July 28, 2024 08:17
@Khader-1 Khader-1 added the buddy review GSoC buddy review needed. label Jul 28, 2024
@Khader-1 Khader-1 changed the title api: Rename StreamMessage to ChannelMessage api: Rename StreamMessage to ChannelMessage Jul 28, 2024
Copy link
Collaborator

@sm-sayedi sm-sayedi left a comment

Choose a reason for hiding this comment

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

Thanks @Khader-1. LGTM. Moving on to the integration review.

@sm-sayedi sm-sayedi added integration review Added by maintainers when PR may be ready for integration and removed buddy review GSoC buddy review needed. labels Jul 28, 2024
@sm-sayedi sm-sayedi requested a review from gnprice July 28, 2024 14:40
@Khader-1 Khader-1 force-pushed the rename-stream-message-to-channel-message branch from 775beb1 to ecaf03a Compare August 1, 2024 05:42
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @Khader-1! Comments below.

I haven't read through the whole second commit in detail; one comment is about how it can be made easier to review.

/// Construct an example stream message.
/// Construct an example stream channel.
Copy link
Member

Choose a reason for hiding this comment

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

"channel message"

StreamMessage streamMessage({
ChannelMessage channelMessage({
Copy link
Member

Choose a reason for hiding this comment

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

Because this function eg.streamMessage has a huge number of references (333 of them, says my IDE), its rename should go in its own commit, separate from editing other things that refer to StreamMessage or "stream messages".

That'd help make the changes easier to review — including self-review to catch things like "example stream channel" noted above.

@gnprice
Copy link
Member

gnprice commented Aug 3, 2024

Closing this PR as part of pushing back the priority of the remaining portion of this issue #631: details at #631 (comment). Thanks again!

@gnprice gnprice closed this Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants