Skip to content

api: Rename ApiNarrowStream to ApiNarrowChannel #847

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
wants to merge 2 commits into from

Conversation

Khader-1
Copy link
Collaborator

Fixes parts of #631

@Khader-1 Khader-1 requested a review from rajveermalviya July 28, 2024 08:12
@Khader-1 Khader-1 added the buddy review GSoC buddy review needed. label Jul 28, 2024
Copy link
Member

@rajveermalviya rajveermalviya 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!
This looks good to me, moving on to integration review.

@rajveermalviya rajveermalviya requested a review from gnprice July 28, 2024 14:35
@rajveermalviya rajveermalviya 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
@Khader-1 Khader-1 force-pushed the rename-api-narrow-stream branch from 7d3fbd6 to 4da6c62 Compare August 1, 2024 05:48
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! See comment below — let's leave this class ApiNarrowStream out of the current round of work on #631.

Comment on lines -37 to 38
class ApiNarrowStream extends ApiNarrowElement {
class ApiNarrowChannel extends ApiNarrowElement {
@override String get operator => 'stream';
Copy link
Member

Choose a reason for hiding this comment

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

An instance of an ApiNarrowElement subclass represents a narrow filter:
https://zulip.com/api/construct-narrow
and generally the subclass ApiNarrowFoo represents narrow filters with the operator "foo".

Here, we're going to want to start representing the operator "channel" and sending that to the server instead of "stream". That's a bit tricky because we can't say "channel" to a server that doesn't understand it, i.e. one older than feature level 250. We've successfully handled the same thing for the pm-with to dm operator; see ApiNarrowDm and its subclasses.

I think I don't want to rename this class without doing that handling, though — I feel like that will make it harder for us to notice that that's something we still need to do. So I'll close this PR for now.

(This is different from the situation with parts of the API that are about data we receive. For those, generally the server is still sending the old name and there's nothing for us to do yet toward speaking the new name with the server; the next steps are on the server side. Here, the server accepts both the old and the new name, so it's done what it can and the next steps are entirely on the client side, for us to start sending the new name instead of the old.)

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