Skip to content

Conversation

@byte-sized-emi
Copy link
Collaborator

@byte-sized-emi byte-sized-emi commented Feb 10, 2024

Implements #180 #181 #182 #183 #184

TODO:

  • User-facing commands
  • Admin-facing commands
  • Testing
  • Permissions (!)

@byte-sized-emi
Copy link
Collaborator Author

byte-sized-emi commented Mar 29, 2024

I've just tested it manually, seems to work well. We should figure out how best to handle errors when for example the user has no study group role - I'd like to avoid replying to the user on discord in every function / when anything fails, as we'd need the context in every function, so it would be nice if we could have either some generic error (like the anyhow and miette) or create a custom error using thiserror, bubbling up errors using normal Result's and replying with them.

Errors thrown by a command function can be caught using an on_error handler (this can do much more, like catch command panics, btw). This means that we can use either of the options - I've tried out miette, and it works beautifully. We need to add the dependency, add a simple error handler (~40 lines of code, because I've added a lot of information), and whenever we want to return an error (that the user should see), we simply do return Err(miette::miette!("Some error here - supports fmt like arguments: {x}"));

@maxwai
Copy link
Collaborator

maxwai commented Apr 4, 2024

I've just tested it manually, seems to work well. We should figure out how best to handle errors when for example the user has no study group role - I'd like to avoid replying to the user on discord in every function / when anything fails, as we'd need the context in every function, so it would be nice if we could have either some generic error (like the anyhow and miette) or create a custom error using thiserror, bubbling up errors using normal Result's and replying with them.

Errors thrown by a command function can be caught using an on_error handler (this can do much more, like catch command panics, btw). This means that we can use either of the options - I've tried out miette, and it works beautifully. We need to add the dependency, add a simple error handler (~40 lines of code, because I've added a lot of information), and whenever we want to return an error (that the user should see), we simply do return Err(miette::miette!("Some error here - supports fmt like arguments: {x}"));

I think the best would be to define our own errors (in an error module) and then handle them in on_error function (put the actual handling in the error module and just call the handler in on_error, keeping it as short as possible)

Edit: for inspiration one can look at how it was done in the python version: https://github.com/marcelropos/HM-DiscordBot/tree/master/core/error

@byte-sized-emi
Copy link
Collaborator Author

I'm gonna guess that this is done so that individual errors can report their own information in a customizable way, with discord-specific ways of formatting?

Also, I think the best way to implement this (with the above presumption) is to use thiserror to get a std::error::Error implementation for a custom error type (which will be the main error type for this project), and then create a method on the error type which spits out a CreateReply. That way, we can use the easy formatting options from thiserror for most of the simple errors, while being able to match on the more complex errors and generate a discord-specific reply.

@maxwai
Copy link
Collaborator

maxwai commented Apr 4, 2024

I'm gonna guess that this is done so that individual errors can report their own information in a customizable way, with discord-specific ways of formatting?

Yes exactly.

to use thiserror to get a std::error::Error implementation for a custom error type, and then create a method on the error type which spits out a CreateReply

Yes sound goods

@byte-sized-emi
Copy link
Collaborator Author

Alright - is it okay if I put this in the scope of this pr? Would be good if I had error handling, and with thiserror, I don't think it's that much work

@maxwai
Copy link
Collaborator

maxwai commented Apr 4, 2024

Yes put it in here

@byte-sized-emi byte-sized-emi marked this pull request as ready for review April 6, 2024 16:43
@byte-sized-emi byte-sized-emi requested a review from maxwai April 6, 2024 16:58
@byte-sized-emi byte-sized-emi changed the title WIP: Command subjects Command subjects Apr 6, 2024
Copy link
Collaborator

@maxwai maxwai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a quick review from the diff. Still need to do local testing and to actually look into the subject.rs file

Comment on lines +7 to +27
pub enum Error {
/// TODO: Actually put a meaningful error in here.
/// needs integration with the mysql_lib module.
#[error("Error with the database, check logs")]
Database,
#[error("Serenity error")]
Serenity(#[from] serenity::Error),
#[error("You are not part of a semester study group")]
UserIsMissingSemesterStudyGroup,
#[error("Invalid subject id: {0}")]
InvalidSubjectId(i32),
#[error("Invalid subject name: {0}")]
InvalidSubjectName(String),
}

impl Error {
/// This can match on specific error types, so that they can be formatted using
/// discord-specific formatting options.
pub fn create_reply(&self) -> poise::CreateReply {
poise::CreateReply::default().content(self.to_string()).reply(true)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error reply should look similar to the old error reply (of course not 1:1). So the errors should have following fields in an embed:

  • Command
  • Cause
  • Solution
  • as a footer the command author and current time

Also, the possibility, if needed, to write something outside of the embed. (Often used for pings)

An Example how it currently looks like:

2024-04-13_08-29

Of course this may go over the scope of the PR so let me know how you want to handle this. I could implement it in another PR, leaving this implementation for now or I could also implement it in this PR directy. Or you may event implement it if you want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to implement it here because there's not a lot of development going on outside this, and the changes here aren't needed for other pr's right now. You can implement it in here if you want to, I'm not sure I know what would be best for users, which is 90% of the implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will do it then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP PR is WIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add subject remove command Add new subject add command Add subject remove command Add subject add command Add subject show command

3 participants