cfallin labeled issue #1056:
The verifier functions have an API that could probably be simplified. Functions which return
VerifierStepResult
must by convention also take an out-paramVerifierErrors
that will contain non-fatal errors.Moreover, the
T
inVerifierStepResult<T>
seems to always be set to()
, so it's unused.It seems the out param is redundant with the error that's present in the
Result
hiddenVerifierStepResult
. I think slightly modifying the interface ofVerifierStepResult
would avoid this out param:
- let the
Ok
type beVerifierErrors
, in case we only have non-fatal errors (they could be called "warnings").- let the
Err
type stay the same (VerifierErrors
too), and contain non-fatal and fatal errors, if there was at least one fatal error.Then we wouldn't need the
errors
outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of thefatal!
etc macros would need to have their ownerrors
variable, but that seems OK.Thoughts?
cfallin labeled issue #1056:
The verifier functions have an API that could probably be simplified. Functions which return
VerifierStepResult
must by convention also take an out-paramVerifierErrors
that will contain non-fatal errors.Moreover, the
T
inVerifierStepResult<T>
seems to always be set to()
, so it's unused.It seems the out param is redundant with the error that's present in the
Result
hiddenVerifierStepResult
. I think slightly modifying the interface ofVerifierStepResult
would avoid this out param:
- let the
Ok
type beVerifierErrors
, in case we only have non-fatal errors (they could be called "warnings").- let the
Err
type stay the same (VerifierErrors
too), and contain non-fatal and fatal errors, if there was at least one fatal error.Then we wouldn't need the
errors
outparam anymore, which looks cleaner and "more Rusty". It might mean that a few users of thefatal!
etc macros would need to have their ownerrors
variable, but that seems OK.Thoughts?
aidenfoxivey commented on issue #1056:
@cfallin This issue seems finished - could it be a candidate for being closed?
cfallin commented on issue #1056:
I don't think the core issue is actually resolved at all; and in the linked PR it mentions that it resolved a subpart of the problem only. This could still be a good starter-issue for someone wanting to do some cleanups.
aidenfoxivey commented on issue #1056:
I don't think the core issue is actually resolved at all; and in the linked PR it mentions that it resolved a subpart of the problem only. This could still be a good starter-issue for someone wanting to do some cleanups.
Ah neat, I see. I'll give it a read. :)
Last updated: Dec 23 2024 at 12:05 UTC