Skip to content

Zulip stream type check #2150

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apiraino
Copy link
Contributor

@apiraino apiraino commented Aug 11, 2025

The Zulip Message API has changed a little bit since we implemented our client. This patch:

  • replaces "private" with "direct" for direct messages (see docs), no functional change.
  • fixes/improves the direct vs. stream check we do when the triagebot server receives a message from the Zulip server. The previous check was not wrong per se but the correct check should be on the message type.
  • Rewrote/fixed and (hopefully) clarified the documentation about testing Zulip stuff

tbh these changes are not really necessary but slightly improve the correctness of our client.

For anyone who has an opinion :)

@rustbot
Copy link
Collaborator

rustbot commented Aug 11, 2025

Failed to set assignee to anyone: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rust-lang rust-lang deleted a comment from rustbot Aug 11, 2025
@rust-lang rust-lang deleted a comment from rustbot Aug 11, 2025
Copy link
Member

@Urgau Urgau left a comment

Choose a reason for hiding this comment

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

Looks okay to me, but I haven't tested it, so I will let others take a look otherwise I will merge in the next few days.

@apiraino
Copy link
Contributor Author

apiraino commented Aug 11, 2025

thanks! Testing this is a bit tedious, but I will do that in the next couple of days

@rustbot author

subject: Option<String>,
/// The type of the message: stream or private.
#[allow(unused)]
Copy link
Contributor Author

@apiraino apiraino Aug 11, 2025

Choose a reason for hiding this comment

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

notice that I also removed the "unused" attribute introduced in 41a793d

@rust-lang rust-lang deleted a comment from rustbot Aug 12, 2025
@apiraino apiraino force-pushed the fixes-zulip-message-struct branch 2 times, most recently from ddeb6f3 to 6765f22 Compare August 12, 2025 10:18
@apiraino apiraino force-pushed the fixes-zulip-message-struct branch from 6765f22 to 0c03e13 Compare August 12, 2025 10:25
@apiraino
Copy link
Contributor Author

apiraino commented Aug 12, 2025

Hmm waiting on the Zulip folks to reply to a question because my tests seems to indicate that Zulip still sends the old payload.

If true, this patch would break everything so I'll put it into draft status to be sure it's not merged.

@apiraino apiraino marked this pull request as draft August 12, 2025 10:37
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.

3 participants