lphillips / SecretHitlerDiscord

A discord bot for the famous board game Secret Hitler
Other
1 stars 0 forks source link

-startgame error checking #24

Closed lphillips closed 3 years ago

lphillips commented 3 years ago

The game needs much improved exception handling and testing. This PR is part of the initial effort to come up with a methodology for solving this problem. Things are still pretty rough, but some of the fundamental problems are solved here, like mocking async calls, and raising exceptions to test exception handling. Posting this very early for discussion and suggestions.

bsmccain commented 3 years ago

I think this framework seems reasonable in general. But I'm wondering if we should investigate the discord.on_error event more before we go too deep into try/catch every api call. There are likely specific cases that we'd want to do that, but there are also going to be more general cases (permission issues) that we don't really need to check everywhere. And a side note about permissions, I don't think you can actually change permissions on an integration, you have to remove it and recreate it. So we should be able to verify them when the bot first connects, then we shouldn't need to verify them again.

lphillips commented 3 years ago

I think this framework seems reasonable in general. But I'm wondering if we should investigate the discord.on_error event more before we go too deep into try/catch every api call. There are likely specific cases that we'd want to do that, but there are also going to be more general cases (permission issues) that we don't really need to check everywhere.

Yeah, that's a really good point. I was already getting concerned about how much exception handling would need to happen. It's probably only really in the state machine that we need to catch exceptions, because we may need to update the state machine in response to the error.

And a side note about permissions, I don't think you can actually change permissions on an integration, you have to remove it and recreate it. So we should be able to verify them when the bot first connects, then we shouldn't need to verify them again.

This is also an excellent point. I much prefer doing things that way to what is currently in this PR.

gerrholz commented 3 years ago

Hello, I just wanted to say that i love how much you guys are improving this bot. I made this in one day for me and my friends and since then i was to lazy to update this. Thank you for making the bot better with your work. I really appreciate that. I also merged your changes into the main repository. They are now accessible to everyone

bsmccain commented 3 years ago

Hello, I just wanted to say that i love how much you guys are improving this bot. I made this in one day for me and my friends and since then i was to lazy to update this. Thank you for making the bot better with your work. I really appreciate that. I also merged your changes into the main repository. They are now accessible to everyone

We're fans of the game and were excited to see you had written this. We had some troubles with people unfamiliar with the game when we tried it out, so we took a look at the code to see if we could improve things. The code has good bones and the game logic all seems solid, so we figured we could work on cleaning it up, especially error cases, to make it a little more user friendly.

It's nice to hear you're happy we're working on it and that you'd like to merge it back into the main repo. We were planning to make PRs back to your repo as we make stable improvements. In the meantime, you might want to wait on merging our changes, we're not necessarily thinking of each PR we make in this repo to be ready for public consumption.

gerrholz commented 3 years ago

Hello, I just wanted to say that i love how much you guys are improving this bot. I made this in one day for me and my friends and since then i was to lazy to update this. Thank you for making the bot better with your work. I really appreciate that. I also merged your changes into the main repository. They are now accessible to everyone

We're fans of the game and were excited to see you had written this. We had some troubles with people unfamiliar with the game when we tried it out, so we took a look at the code to see if we could improve things. The code has good bones and the game logic all seems solid, so we figured we could work on cleaning it up, especially error cases, to make it a little more user friendly.

It's nice to hear you're happy we're working on it and that you'd like to merge it back into the main repo. We were planning to make PRs back to your repo as we make stable improvements. In the meantime, you might want to wait on merging our changes, we're not necessarily thinking of each PR we make in this repo to be ready for public consumption.

That's awesome to hear. Its really good that you want to make it more user-friendly. As i mentioned before, i made this bot for me and my friends so it was not necessary to have good error handling since i could just look up in the console what was going wrong. But for other people this might not be the case. I also thought of making a .exe file for the bot, when its more improved so people could just download it on their computer and start running it without having to install discord.py and the other packages. What do you think of that?

bsmccain commented 3 years ago

That's awesome to hear. Its really good that you want to make it more user-friendly. As i mentioned before, i made this bot for me and my friends so it was not necessary to have good error handling since i could just look up in the console what was going wrong. But for other people this might not be the case. I also thought of making a .exe file for the bot, when its more improved so people could just download it on their computer and start running it without having to install discord.py and the other packages. What do you think of that?

I had the thought of making a docker image for it. An executable for windows and mac would also be good, we should look into that.

lphillips commented 3 years ago

I'm closing this PR in favor of a permission verification step during set up.