Closed SilentByte closed 4 years ago
Hmm I'm thinking if we really want this to be implicit or explicit. i.e. some user might also have:
enum MyResult<T, E> {
Succeed(T),
Failed(E),
}
but won't have the needed map
, map_err
functions.
Hmm I'm thinking if we really want this to be implicit or explicit. i.e. some user might also have:
enum MyResult<T, E> { Succeed(T), Failed(E), }
but won't have the needed
map
,map_err
functions.
But maybe if any user of this crate has this problem then we'll wait for a complaint and think of a fix? what do you think?
I don't have experience with proc macros, but wouldn't it be possible that if ok
and/or err
are specified, e.g. #[logfn(ok = "INFO", err = "ERROR")]
, that the macro just expects the return value to be able to be mapped to Ok
and Err
and generates the code accordingly? If that's not the case, the compiler could just throw an error (due to invalid code generated).
But maybe if any user of this crate has this problem then we'll wait for a complaint and think of a fix? what do you think?
I agree, I don't think this change will have a breaking impact on a lot of user's code.
Sure it is. the main question here is what should be the default case for #[logfn(INFO)]
. should it try to infer if it's Result using the postfix or not.
Hm good question. I think if ok
/err
are not specified, it should just normally log the return value and not treat it as anything special (even if it is Result) If ok
/err
are specified, then it should behave just like with this PR.
The whole point for the current is_result_type
is to log it "better" by default without the need for explicit logging
What do you think about leaving the implicitly but forcing to treat it as Result
if either ok
or err
is specified?
I do want some good default for the lazy #[logfn]
.
How do you feel about leaving it as is but if you add Ok/Err
I ignore my checks and assume it's a Result type.
@SilentByte what do you think about this: https://github.com/elichai/log-derive/commit/841177536549aeb6377c3f0ddea35032282bab32
EDIT: ha, it breaks some of my examples, because apparently I used the ok = "X"
in examples that don't return a Result heh.
How do you feel about leaving it as is but if you add
Ok/Err
I ignore my checks and assume it's a Result type.
I agree. I believe the way it works now is sensible. #[logfn(Info)]
currently logs both Ok
and Err
with the Info level, as it should. When ok
/err
are specified, the macro should force the return type to support Ok/Err, and error out if this isn't the case.
Thank you for the contribution!
It is quite common to define custom error enums and combine them with a Result type alias, e.g.:
However, this isn't being caught by log-derive (a similar issue has been https://github.com/elichai/log-derive/pull/6). This PR changes the
is_result_type
function to accept all types that end in Result, which seems to cover additional cases.I'm not sure if this change has any negative implications, but it works nicely for my use-case. :smile: