Comments (9)
We can't add a Fromstd::error::Error implementation for Error to return Error::User, because it would conflict with the From for T impl from the standard library (as Error implements std::error::Error).
I don't think such a trait impl is needed. The generated command code can embed the user code like this:
let result: Result<(), USERERROR> = async {
// USER CODE HERE
}.await;
result.map_err(FrameworkError::User)
from framework.
Currently, when a user gives too many arguments to a command, or too few, or their input was malformed,
serenitythe framework bubbles up the error as aError::User
.
This is intended. Argument parsing is part of the command's code that's first executed before running the actual body of the command. Failure to parse arguments constitutes as a user error, since it is the user's responsibility to parse arguments (the #[command]
macro just provides syntax sugar to generate code the user might have written themselves).
To clarify, dispatch errors originate from the framework, user errors from the commands.
from framework.
Yeah I can see why it happens. I can't quite interpret what you think of the proposal though - do you agree that this behavior is confusing and a designated error enum variant might be beneficial, or is this a "working as intended" situation?
In other words, would a PR about this issue be accepted?
from framework.
The latter, "working as intended." The current error enum makes the origin of the errors from either the framework or commands clear, and allows the two to be handled separately:
fn handle_dispatch_error(err: DispatchError) {
// ....
}
// Assuming the user defined a custom error type called `MyError`
fn handle_my_error(err: MyError) {
// ...
}
let error = ...;
match error {
Error::Dispatch(err) => handle_dispatch_error(err),
Error::User(err) => handle_my_error(err)
}
If the user error was merged into DispatchError
, the above match
would still be possible, but more ugly, among other ways to handle errors.
Therefore, I don't believe that this is an issue that ought to be fixed. Besides that, how would we distinguish argument errors from other errors returned from commands? By default, the error type for commands is Box<dyn std::error::Error>
, but the user can change that to something else. We could perhaps add a wrapper error type for argument errors and other errors for commands. However, that would be an ugly solution for the user and the framework, asserting my point that argument errors should stay as user errors.
from framework.
I see your concern about error handling being more difficult with the Error enum variants merged. I think there is a misconception here. My idea was not to merge the variants, but to add a new variant.
Either to Error:
pub enum Error {
Dispatch(DispatchError),
Argument(ArgumentError<Box<dyn std::error::Error>>), // <--
User(E),
}
or to DispatchError:
pub enum DispatchError {
NormalMessage,
PrefixOnly(String),
InvalidCommandName(String),
CheckFailed(String, Reason),
ArgumentError(ArgumentError<Box<dyn std::error::Error>>), // <--
}
(Not sure yet which one of the two is more ergonomic for the user. Also, Box<Error>
because parse implementations may return all kinds of different error types).
Funnily enough, the motivation behind this change is of the same kind as the concern you expressed: merging argument parsing errors and user-thrown errors into the same Error::User
variant makes precise error handling more difficult.
Have I understood and addressed your concern?
from framework.
I understood your request to separate argument errors from Error::User
, and to place them into Error::Dispatch
, but
or to DispatchError:
That is what I meant by merging into DispatchError
, that should belong entirely to the framework. Argument errors from commands are not framework errors. Adding a new variant to Error
would be fine, but how would argument errors be differentiated from other errors returned from commands? The current definition of CommandResult
is type CommandResult<E = DefaultError> = Result<(), E>;
, that doesn't leave room to separate argument errors from other errors when a user defines their own error type. As I said, we could have a wrapper like so:
enum CommandError<E> {
Argument(ArgumentError<Box<dyn std::error::Error>>),
Normal(E),
}
and use it instead of a plain E
in Result<(), E>
from the type alias above. This will work, but again, I don't like it. This wrapping should be done by the user with their own error type. Even if a user just depends on the default error type (Box<dyn std::error::Error>
), they can downcast to ArgumentError
and handle argument errors separately. I wouldn't have written ArgumentError
otherwise if I hadn't considered this scenario.
from framework.
This wrapping should be done by the user with their own error type. Even if a user just depends on the default error type (Box), they can downcast to ArgumentError and handle argument errors separately.
Ah, now I understand how you envisioned users to handle argument parsing errors. Either call .downcast::<ArgumentError>
on the Box<dyn Error>
error, or swap in a custom error type with a From<ArgumentError>
implementation, that allows later retrieval of the stored ArgumentError instance. That makes sense.
Adding a new variant to Error would be fine, but how would argument errors be differentiated from other errors returned from commands?
The generated function would yield a Result<(), FrameworkError<USERERROR>>
instead of Result<(), USERERROR>
. The function contents would need to be adjusted accordingly to correctly wrap argument parse errors in FrameworkError::Argument
and user-thrown errors in FrameworkError::User
. I have created a quick mock-up function to test this approach and it successfully compiled.
What I personally like about this approach in comparison to the other two you presented (downcasting and custom error type):
- it's easily discoverable: it's clear to users how they are supposed to handle argument parsing errors, which is the main problem I had. Not everyone knows about downcasting, and it is not at all obvious to use downcasting or custom error types to handle ArgumentErrors, particularly to people who don't know how the proc-macro generated code works
- it doesn't require users to adapt their error type to fit serenity's needs (read: provide a
From<ArgumentError>
implementation for their custom error type). This point is quite subjective but it feels unfortunate to me that users need to be aware of hidden code injected by serenity possibly throwing an ArgumentException, just in order to design their own error type. That feels like serenity is shifting responsibility over to the user when it should really handle its responsibilities itself.
By the way, to be clear: It is not my intention to force through my subjective opinion on this topic. I merely want to ensure that my idea and its (dis)advantages are fully understood and considered before the accept/reject decision is made.
from framework.
The generated function would yield a
Result<(), FrameworkError<USERERROR>>
instead ofResult<(), USERERROR>
. The function contents would need to be adjusted accordingly to correctly wrap argument parse errors inFrameworkError::Argument
and user-thrown errors inFrameworkError::User
. I have created a quick mock-up function to test this approach and it successfully compiled.
Looking from that angle, I like it, actually. I disagreed with your original proposal a lot, but reusing Error
rather than creating a wrapper error type or integrating argument errors into DispatchError
can simplify things. There's a slight problem, though. We can't add a From<std::error::Error>
implementation for Error
to return Error::User
, because it would conflict with the From<T> for T
impl from the standard library (as Error
implements std::error::Error
).
from framework.
Ah, then I have no more issues with this. You're free to open a pull request.
from framework.
Related Issues (13)
- Implement builtin plain-text and embed help commands
- Implement localisation utilities
- &D as user data instead of Arc<RwLock<D>>?
- Allow serenity model types in argument parsing system HOT 6
- Contain &Message in FrameworkContext HOT 2
- Framework generic parameter deduction fails with type alias HOT 1
- Roadmap for v0.1.0
- Argument parsing for required and optional arguments should have a flexible order HOT 1
- Add a `OneOf` type for command arguments HOT 2
- Implement `#[check]`
- Implement command arguments
- Port buckets from the original standard framework
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from framework.