Skip to content
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

add --topic arg to rss-bot script #747

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

dimisjim
Copy link

No description provided.

Copy link
Contributor

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@dimisjim Thanks for submitting this feature 👍

I expect existing users could benefit from this, but would also appreciate the option of retaining the existing behavior.

In addition, it would be useful if you could adjust the documentation to take into account any new option.

"--topic",
dest="topic",
help="The Stream Topic to which to send RSS messages.",
default="stream events",
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the default behavior which currently exists; is there a reason for this?

Copy link
Author

Choose a reason for hiding this comment

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

I've removed the default value via: 4de0952

Is my assumption correct that it will now fallback to data.feed.title or feed_url if no --topic arg is specified?

@@ -241,7 +248,7 @@ for feed_url in feed_urls:
# entries in reverse chronological order.
break

feed_name = data.feed.title or feed_url # type: str
feed_name = opts.topic
Copy link
Contributor

Choose a reason for hiding this comment

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

While this change sets the topic, this doesn't preserve the existing behavior, when a fixed topic is not specified.

Copy link
Author

Choose a reason for hiding this comment

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

should be addressed via: 4de0952

@dimisjim dimisjim requested a review from neiljp February 25, 2022 07:48
@@ -42,6 +42,10 @@ You can customize the location on the feed file and recipient stream, e.g.:

/usr/local/share/zulip/integrations/rss/rss-bot --feed-file=/path/to/my-feeds --stream=my-rss-stream

optionally, you can also specify the name of the stream topic, e.g.:

/usr/local/share/zulip/integrations/rss/rss-bot --feed-file=/path/to/my-feeds --stream=my-rss-stream --topic=my-topic-name
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe specify an example topic containing spaces, e.g. --topic="topic name", since that's more typical naming style.

Copy link
Author

Choose a reason for hiding this comment

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

indeed: 3574ed6

@@ -248,7 +251,7 @@ for feed_url in feed_urls:
# entries in reverse chronological order.
break

feed_name = opts.topic
feed_name = opts.topic or data.feed.title or feed_url # type: str
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I would expect feed_url to often be too long; do we want to clamp too long topics here in the client code?

Copy link
Author

Choose a reason for hiding this comment

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

not sure what your suggestion is here really.

data.feed.title or feed_url was already here before my PR

@dimisjim dimisjim requested a review from timabbott March 4, 2022 11:57
@i-ky
Copy link
Contributor

i-ky commented Nov 8, 2023

The feature has been implemented in #785.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants