Slack bridge: Implement multiple channels bridges. #739

rht · 2021-11-28T06:24:31Z

No description provided.

zulip/integrations/bridge_with_slack/run-slack-bridge

rht · 2022-09-29T09:56:27Z

@PIG208 thank you for the review!

rht · 2022-09-29T10:24:19Z

I have implemented the requests in the 3rd commit.

PIG208

Except for a minor issue and the CI failure, it LGTM! Thanks.

PIG208 · 2022-09-29T12:43:04Z

zulip/integrations/bridge_with_slack/run-slack-bridge

    msg: Dict[str, Any], zulip_to_slack_map: Dict[str, Any], bot_email: str
-) -> Tuple[bool, str]:
+) -> Union[str, None]:


We can use Optional[str] here.

I intentionally didn't use Optional because this is different from the situation with arg=None in function arguments. It'd be better with str | None, but this is not supported in Python <3.10 unless there is from __future__ import annotations. See https://peps.python.org/pep-0604/.

I agree that the future syntax is more explicit (if unavailable), but while the choice is kinda arbitrary, I thought Zulip had mainly settled on Optional in these cases? The typing is exactly equivalent. I don't see the relevance of arg=None cases.

OK I will just do what is done in zulip/zulip.

neiljp

@rht Always interested to see more bridge extensions 👍

The other review pushed it onto my radar, this is only from the perspective of reading the code.

neiljp · 2022-09-29T17:59:53Z

zulip/integrations/bridge_with_slack/run-slack-bridge

    msg: Dict[str, Any], zulip_to_slack_map: Dict[str, Any], bot_email: str
-) -> Tuple[bool, str]:
+) -> Union[str, None]:


I agree that the future syntax is more explicit (if unavailable), but while the choice is kinda arbitrary, I thought Zulip had mainly settled on Optional in these cases? The typing is exactly equivalent. I don't see the relevance of arg=None cases.

neiljp · 2022-09-29T18:06:28Z

zulip/integrations/bridge_with_slack/run-slack-bridge

@@ -18,23 +18,25 @@ ZULIP_MESSAGE_TEMPLATE = "**{username}**: {message}"
 SLACK_MESSAGE_TEMPLATE = "<{username}> {message}"


-def check_zulip_message_validity(
+def get_zulip_message_slack_channel(


This may be a difference in how (human) languages name variables, but I'd choose something closer to slack_channel_for_zulip_message.

neiljp · 2022-09-29T18:10:50Z

zulip/integrations/bridge_with_slack/run-slack-bridge


    stream_name = msg["display_recipient"]
    topic_name = msg["subject"]
-    stream_topic = stream_name + "-" + topic_name
+    stream_topic = f"{stream_name}-{topic_name}"


The f-string formatting is an improvement, but reading this I wondered if you'd considered using a stream-topic tuple in your dicts instead?

Converted to tuple.

neiljp · 2022-09-29T18:16:11Z

zulip/integrations/bridge_with_slack/run-slack-bridge

+    print("MAKE SURE THE BOT IS SUBSCRIBED TO THE RELEVANT ZULIP STREAM(S)!")
+    print("MAKE SURE THE BOT IS SUBSCRIBED TO THE RELEVANT SLACK CHANNEL(S)!")


Good to include, but I think as a user, seeing these two lines 'shouting' at me with a lot of common text, I might not notice that they're subtly different. Maybe an (expanded) formatting on three lines relating to 1) common subscribe text 2) zulip stream(s) 3) slack channel(s) ?

I have combined the 2 lines into 1 sentence.

neiljp · 2022-09-29T18:21:53Z

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py

+    # Mapping between Slack channels and Zulip stream-topic's.
+    # You can specify multiple pairs.


I'd be inclined to give a 'multiple pair' as an example, so the format is obvious. Or maybe more than one "pairs" section, with some commented out, as alternative examples.

Then maybe even multiple examples of how it could be used. eg. multiple slack channels -> many topics in one stream, etc

rht · 2022-09-30T12:02:32Z

I have implemented the requests and tidied the commits.

neiljp · 2022-10-01T00:42:48Z

@rht I'm not sure how much detail is expected here before merging, but other than the checks above (one needing fixing - static-analysis), has this been tested and if so how?

rht · 2022-10-01T01:50:19Z

The static analysis test has been fixed.

has this been tested and if so how?

I tested this on a live Slack workspace and CZO. I have retested just now, just in case if the refactor causes any regression.
See https://chat.zulip.org/#narrow/stream/7-test-here/topic/.3C-.20slack-bridge.20general and https://chat.zulip.org/#narrow/stream/7-test-here/topic/.3C-.20slack-bridge.20general

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py

timabbott · 2022-10-05T18:24:37Z

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py

+    "pairs": {
+        "C5Z5N7R8A -- <Slack channel; must be channel id>": {
+            "stream": "test here",
+            "topic": "<- slack-bridge",


Is there a way to specify "Use the slack channel name as the topic" here? Could be nice to support substitution of the Slack channel name with a syntax like "slack: {channel_name}.

The other feature that might be nice is to support the special key default to be used for any Slack channels not listed here, so you can just have a single map for a whole Slack instance. But might be better as a follow-up feature.

Could be nice to support substitution of the Slack channel name with a syntax like "slack: {channel_name}.

For a few number of pairs, users can specify that pattern by themselves explicitly. Adding this feature adds more complication to what the config can be, and requires more documentation

I haven't encountered >40 Slack channels pairing use case. The original use case is to bridge Julia Slack to Julia Zulip for content preservation, and they wanted to do so for only a few of the channels they are comfortable with being preserved. If this is needed soon, I can do it a follow-up PR.

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py

timabbott · 2022-10-19T22:17:20Z

Merged, thanks @rht!

zulipbot added the size: L label Nov 28, 2021

timabbott requested a review from PIG208 November 30, 2021 02:02

rht force-pushed the slack-bridge branch from 89e75ac to 62bdfd5 Compare May 24, 2022 01:30

PIG208 reviewed Sep 28, 2022

View reviewed changes

rht force-pushed the slack-bridge branch from 62bdfd5 to 1ee14e5 Compare September 29, 2022 10:22

rht force-pushed the slack-bridge branch 2 times, most recently from a6dec61 to ff88713 Compare September 29, 2022 10:51

PIG208 reviewed Sep 29, 2022

View reviewed changes

neiljp reviewed Sep 29, 2022

View reviewed changes

rht force-pushed the slack-bridge branch from ff88713 to 2bae16c Compare September 30, 2022 12:01

rht force-pushed the slack-bridge branch 2 times, most recently from 3b161d2 to 3bb52f8 Compare October 1, 2022 01:41

timabbott reviewed Oct 5, 2022

View reviewed changes

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py Outdated Show resolved Hide resolved

timabbott reviewed Oct 5, 2022

View reviewed changes

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py Outdated Show resolved Hide resolved

timabbott reviewed Oct 5, 2022

View reviewed changes

rht force-pushed the slack-bridge branch from 3bb52f8 to 5149645 Compare October 6, 2022 00:26

timabbott reviewed Oct 19, 2022

View reviewed changes

zulip/integrations/bridge_with_slack/bridge_with_slack_config.py Outdated Show resolved Hide resolved

rht added 2 commits October 18, 2022 21:48

Slack bridge: Implement multiple channels bridges.

ada5538

Slack bridge: Inform user to subscribe their Slack bot.

8aff96c

rht force-pushed the slack-bridge branch from 5149645 to 8aff96c Compare October 19, 2022 01:48

timabbott merged commit 582e973 into zulip:main Oct 19, 2022

rht mentioned this pull request Oct 22, 2022

Slack bridge: Add option that let the bridge auto-specify Slack channel name as Zulip topic #769

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slack bridge: Implement multiple channels bridges. #739

Slack bridge: Implement multiple channels bridges. #739

rht commented Nov 28, 2021

rht commented Sep 29, 2022

rht commented Sep 29, 2022

PIG208 left a comment •

edited

Loading

PIG208 Sep 29, 2022

rht Sep 29, 2022

neiljp Sep 29, 2022

rht Sep 30, 2022

neiljp left a comment

neiljp Sep 29, 2022

neiljp Sep 29, 2022

neiljp Sep 29, 2022

rht Sep 30, 2022

neiljp Sep 29, 2022

rht Sep 30, 2022

neiljp Sep 29, 2022

rht commented Sep 30, 2022

neiljp commented Oct 1, 2022

rht commented Oct 1, 2022

timabbott Oct 5, 2022 •

edited

Loading

rht Oct 6, 2022

timabbott commented Oct 19, 2022

		print("MAKE SURE THE BOT IS SUBSCRIBED TO THE RELEVANT ZULIP STREAM(S)!")
		print("MAKE SURE THE BOT IS SUBSCRIBED TO THE RELEVANT SLACK CHANNEL(S)!")

		# Mapping between Slack channels and Zulip stream-topic's.
		# You can specify multiple pairs.

Slack bridge: Implement multiple channels bridges. #739

Slack bridge: Implement multiple channels bridges. #739

Conversation

rht commented Nov 28, 2021

rht commented Sep 29, 2022

rht commented Sep 29, 2022

PIG208 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neiljp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rht commented Sep 30, 2022

neiljp commented Oct 1, 2022

rht commented Oct 1, 2022

timabbott Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Oct 19, 2022

PIG208 left a comment •

edited

Loading

timabbott Oct 5, 2022 •

edited

Loading