On Stream Review

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L35-L35

It's helpful to name classes in a way that conveys their context rather than the components of the class. For example, here it looks like this view controls something around getting players(?). Maybe name it something like PlayerConfigView or something like that

[braddotcoffee]

Similar here, would be useful to rename this to "add_player_button" and "remove_player_button" https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L66-L67

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L81-L86

Can early return here to un-nest this block of code, improving readability

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L93-L106

This block of code is a little hard to follow. You might be better off breaking this into helper functions of some kind for each "branch". This way when a new contributor is reading to get an understanding of the flow, they get an english description of what happens in each case. Then you drill down into the semantics of what happens.

if self.needed == 0:
    return await start_game()

await interaction.response.send_message( 
     "You've been added to the pod! I'll ping you when there's enough " 
     "for your game.", 
     ephemeral=True, 
 ) 

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L109-L110

nit: Function / method definitions go inside the function

def your_function():
    """Your documentation"""
    pass

[reference]

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L112-L116

You shouldn't tell the user they've been removed from the pod until it's been done successfully. Here you tell them and THEN remove them - reverse the order

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L111

Easier to read if you invert this.

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L123-L127 It's really hard to see your error handling logic since it's way down at the bottom

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L117-L118

This is really unintuitive to read. I think pulling this out into a sorta "self destruct" "cleanup" type of method that's along the lines of self.end_queue() might be good.

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L139-L152

There's no need to throw to your own except if you don't need to exit out of deeply nested logic. Here your goal is just to return a message to the user and exit, so you don't need to make this an error. Just return the message and cleanly exit

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L144-L148

Not sure what this is, probably can be removed

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L158

nit: this comment is unnecessary and also not exactly true. This sends the view / embed to the Discord client, it's not exactly "storing" it somewhere. "Store" tends to imply putting something into a database or some other long-term-storage mechanism

[braddotcoffee]

https://github.com/sinaghandi/PodFinder/blob/21cf186e83a0127c40d2f4f99aaf3b5e77df85ab/main.py#L160

Firstly, view doesn't appear to be globally defined in any way so "storing the message" on this object has no real effect. As soon as this line is run, view leaves scope and disappears from existence

def your_method():
    view = View()
    view.bot_message = message
    print(view) # <- view exists
print(view) # <- view does not exist, you will get a NameError or something along those lines

Even if this did work, this is considered bad practice for two reasons - one of them is ideological and one of them is functional.

Functional

If you were to store a global variable "view" that you keep referencing, then each time that a new instance of the command is run you will overwrite this variable.

!command instance1
!command instance2

Now when instance1 has an interaction on its view, it will be referencing the message from instance2. To achieve this functionality you would need a way to "look up" the message using the discord client. This is why RoboBanana has a database associated with it - sometimes we store message IDs and use those IDs to look up messages later.

In your case this should be entirely unnecessary. When someone clicks on your view, the interaction.message (or whatever the property is called) will reference the message that you're currently trying to store on this view.

Ideological

You shouldn't try to do this because you're attempting to modify the definition of a class that's outside of your control. View is defined by Discord.py and does not contain this attribute. You should instead be making a wrapper class that holds both the view and any additional attributes that you need:

class SomeClass:
    def __init__(self, view: View):
        self.view = view
        # The rest of your stuff

Then you own the entire lifecycle of this object and don't have to rely on Discord.py never modifying the contents of your monkey-patched attribute.

---

EDIT

I was thinking about this earlier today when I realized that during the live review I forgot about the fact that in this section of code, view is not the DiscordPy View class, but your own custom EmbedView class. We actually do a similar thing of "putting stuff on the class to remember for later" in RoboBanana

class PayoutPredictionView(View):
    def __init__(self, option_one: str, option_two: str, client: Client) -> None:
        super().__init__(timeout=None)

        self.option_one = option_one
        self.option_two = option_two
        self.client = client

[reference]

Here we store a Discord client for later use.

I stand by the fact that for your use, I do not believe you need to store the message onto the view. That being said, I believe this would functionally work and is not an antipattern. I wanted to make sure I correct that to clarify that the problem I was referencing is with modifying classes you do not own the definition of. You do own the definition of EmbedView, so you are good to go 😎