Red-DiscordBot icon indicating copy to clipboard operation
Red-DiscordBot copied to clipboard

[Streams] Save/remove/check custom message(s) for each streamer registered

Open KianBral opened this issue 5 years ago • 4 comments

Type

  • [ ] Bugfix
  • [ ] Enhancement
  • [x] New feature

Description of the changes

Worked alongside @willkang-1 (kaesd #6455 on discord) for this PR

This should hopefully resolve #3824, should also fix #4984

We added a few new functions/variables to the streams cog that allow users to add custom mention and no-mention stream alert messages for every individual streamer.

Commands Added

[p]streamset message streamer <streamer_name> <mention|nomention> [if_mention: audience] <message> Allows users to set a custom message for an already-registered streamer, select between mention/no-mention, and give them the optional argument for who to mention, if mention is selected.

[p]streamset message check <streamer_name> Returns the mention/no-mention messages for the input streamer, and notifies if the streamer does not have one of or both of the messages. Will also notifies user if the streamer is not registered in the server.

[p]streamset message remove <streamer> <mention|nomention|both> Removes the mention/no-mention (or both) messages for the input streamer. Notifies user only if streamer is not registered in the server.

Additionally, we added functions and variables to add and remove attributes to streamer objects or dictionaries. Corresponding logic was added to check whether or not a streamer has a custom message, and if not the server will just print the default for the server.

There was an initial commit that was accidentally from another branch. It was reverted in the last commit since we only just realized it, so please ignore it as it does not make any changes, sorry about that!

This was a relatively big and confusing feature to integrate. Please let me or @willkang-1 know if we overlooked anything, made things too complicated, or didn't follow any common conventions that we should have. Also feel free to ask about our implementation.

KianBral avatar Dec 12 '20 02:12 KianBral

Hello @KianBral, thank you for your PR! I'd like to point out certain problems with your PR that needs certain fixes:

  • At line 638, 639, 650, 695, etc., you are using special methods to get, set and delete attributes. This isn't the way to go, you should rather use the builtin commands provided by Python:
# Original
class.__getattr__("attribute")
# Become
class.attribute

# Original
class.__setattr__("attribute", value)
# Become
class.attribute = value

# Original
class.__delattr__("attribute")
# Become
del class.attribute
  • Your strings do not respect i18n when using .format, as the .format should be outside the _() pairs, not inside:
# Incorrect
_("My string".format(format_data=""))

# Correct
_("My string").format(format_data="")
  • A personal point is that you should use f-string rather than format in this context, they are the modern way to go, but you can keep .format if you'd like.
  • When using .format, you should use keyword arguments rather than positional arguments, like this:
# Positional
"My {}".format(stuff)

# Keyword
"My {secret}".format(secret=stuff)

This is in order to improve clarity of arguments you're passing.

  • I think it's better that the information you are passing in the streamer_info attribute should go directly into the Stream class (Available at redbot.cogs.streams.streamtypes)

madebylydia avatar Feb 12 '23 17:02 madebylydia

Hello @KianBral, are you still interested to work on this PR?

madebylydia avatar Dec 29 '23 12:12 madebylydia

@Jackenmen This PR is stalling, but I would like this contribution merged. What can I do about it, so this can get merged, can I re-use this work and open my own PR? I'm unsure about how this is falling under the license if I reuse the code.

madebylydia avatar Jul 05 '24 15:07 madebylydia

You need to keep commit authorship so that when we merge it from your PR, it is attributed to all its authors, not just you.

Jackenmen avatar Jul 05 '24 15:07 Jackenmen