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

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

Open
wants to merge 12 commits into
base: V3/develop
Choose a base branch
from

Conversation

KianBral
Copy link
Contributor

@KianBral KianBral commented Dec 12, 2020

Type

  • Bugfix
  • Enhancement
  • 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 KianBral requested a review from palmtree5 as a code owner December 12, 2020 02:25
@Flame442 Flame442 added Category: Cogs - Streams This is related to the Streams cog. Type: Feature New feature or request. labels Jan 8, 2021
@Jackenmen Jackenmen added this to the 3.4.7 milestone Jan 20, 2021
@madebylydia
Copy link
Contributor

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
Copy link
Contributor

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

@madebylydia
Copy link
Contributor

@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.

@Jackenmen
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Cogs - Streams This is related to the Streams cog. Type: Feature New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Streams] Ability to utiltize more role mentions [Streams] Save a message for each streamer registered
4 participants