Skip to content

api: Rename to ChannelDestination and ChannelPostPolicy from "stream" #846

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

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

Khader-1
Copy link
Collaborator

Fixes parts of #631.

This focuses on two models:

  • ChannelDestination
  • ChannelPostPolicy

@Khader-1 Khader-1 requested a review from sm-sayedi July 28, 2024 08:07
@Khader-1 Khader-1 added the buddy review GSoC buddy review needed. label 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! 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:31
@Khader-1 Khader-1 force-pushed the rename-stream-destination branch from 11e3319 to 0bd2a12 Compare August 1, 2024 05:50
@gnprice gnprice changed the title api: Rename stream to channel api: Rename to ChannelDestination and ChannelPostPolicy from "stream" Aug 2, 2024
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! Merging the ChannelPostPolicy / StreamPostPolicy part of this.

On ChannelDestination, see below.

(The PR branch also contained #847. It's fine to stack one PR atop another one whenever that's helpful, but do be sure to mention it in the PR description because it's important context for reviewers. Since I closed #847 for reasons described there, I'm dropping those commits here too.)

Comment on lines 186 to 187
StreamDestination() => {
ChannelDestination() => {
'type': RawParameter('stream'),
Copy link
Member

Choose a reason for hiding this comment

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

This is a similar story to #847 — StreamDestination describes part of our outgoing API where we say "stream", and so I'd like to package the rename together with starting to say "channel" in what we send. The server does accept "channel" here:
https://zulip.com/api/send-message#parameter-type

@gnprice gnprice force-pushed the rename-stream-destination branch from 0bd2a12 to 1964388 Compare August 2, 2024 23:59
@gnprice gnprice merged commit 1964388 into zulip:main Aug 2, 2024
1 check passed
@gnprice
Copy link
Member

gnprice commented Aug 2, 2024

FTR since GitHub makes dropped commits hard to find: that StreamDestination / ChannelDestination commit was:
9a5eea0 api [nfc]: Rename StreamDestination to ChannelDestination

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