MenuDocs / Pyro

A Python Discord Bot
Apache License 2.0
26 stars 6 forks source link

Database integration #5

Closed Skelmis closed 4 years ago

Skelmis commented 4 years ago

@MonAaraj @MelodicAlbuild preview the code, I expect the help command to change around with better parsing from M.A or myself sometime soon, as well as looking into get the Document class optimized and possibly removing the pointer functions. Throw all the feedback below and don't merge it yet. Much appreciated!

A p.s. I ran all the files through Black on last commit

ribosomerocker commented 4 years ago

Overall very good. Just make sure to look at my code reviews and when you've fixed them we can get this merged, thanks for running this in black by the way! 👍

ribosomerocker commented 4 years ago

@Skelmis also, once you tested if Command.usage returns the usage for the command even if none has been set, if it does, then tell me in my pr for me to merge, if not, then tell me exactly what happens and we'll both work it out 👍

Skelmis commented 4 years ago

I'm aware that command.usage returns None if it is not explicitly set in the decorator. A possible way around this, if we do not wish to need to define command.usage could be to parse the args ourselves and determine what they are? But i think defining any args with usage may be easier, I am not sure because I have not looked into it @MonAaraj

So, if not set. command.usage will be None, which is where this line comes from https://github.com/MenuDocs/Pyro/blob/Database-Integration/cogs/help.py#L106

ribosomerocker commented 4 years ago

We definitely can manuver over it

ribosomerocker commented 4 years ago

Also, please add mongo/motor/whagever you used to install the mongodb module's name into requirements.txt in the same format I did, module_name>=current_version

Skelmis commented 4 years ago

I've looked into the usability of something besides command.usage and, in terms of simplicity for the help command that is the easier way to go about it. A different way to do it could be with the inspect module, but that requires passing in the function to inspect etc whereas if we just define command.usage for command decorators it is what would appear to be the simpler route? Thoughts going forward?

ribosomerocker commented 4 years ago

The problem is that human error can easily happen while declaring usage and making automated usage by using inspect is better we could do that

ribosomerocker commented 4 years ago

Also on your newest commit i really dislike that if message.author is self.bot id or something line that checks if the message sender is the bot, could you make it so that it returns if the message is a bot from the very beginning?

if message.author.bot:
    return
Skelmis commented 4 years ago

Discord.Py process_commands should ignore bots regardless, besides ourselves iirc. But will do

ribosomerocker commented 4 years ago

Also apparently this branch has conflicts, can you see whats wrong? try to use git rebase --interactive development while you're switched on your branch?

ribosomerocker commented 4 years ago

@Skelmis can I get some quick stuff on the reviews and the conflicts before I start working in-case i do something you dont want me to?

Skelmis commented 4 years ago

I think I have a solution to the sub command problem, give me a few minutes to implement it

Skelmis commented 4 years ago

That should be all from me, over to you MA unless there's anything further

ribosomerocker commented 4 years ago

@Skelmis currently getting an error whenever i ping the bot about __get_raw, im needin to go so can u take care of it for a bit?

ribosomerocker commented 4 years ago

remember to git pull -a

Skelmis commented 4 years ago

I noticed that issue, its due to how private class methods are called. it is fixed in my next commit Should be -> _Document__get_raw

ribosomerocker commented 4 years ago

@Skelmis ready for merge after you review.

Skelmis commented 4 years ago

Im happy with it all, but just curious as to why help on _ready is between two methods when its part of dpy listeners so I think it would be better to come after them instead

ribosomerocker commented 4 years ago

@Skelmis dun did it

Skelmis commented 4 years ago

Safe as, im happy if you want to merge it then