ResultOrErrors deconstruct #217
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Drizin
added a commit
to Drizin/FluentResults
that referenced
this pull request
Aug 3, 2024
- I think it doesn't make sense to deconstruct both isSuccess/isFailure (just noisy, one would be enough and promoves uniform usage patterns) - If we're just interested in that specific property (success or failure, without getting the possible Result or IErrors) then we can just get that property - So if you're using a destructor on `Result<TValue>` you NEED to get TValue and optionally List<IError>. - For the non-generic Result (the one that does NOT hold an underlying TValue) the destructor `<bool isSuccess, List<IError> errors>` was moved down from the base class Refs altmann#217
Result<TValue> now has a new deconstruct (golang-style style): `Deconstruct(out TValue value, out List<IError> errors)` Returning isSuccess and isFailed were redundant since we can infer success/failure from non-null values in `TValue value` and `List<IError> errors`. Breaking change: anyone who was using Result<TValue> and was using the descontruct that returns bool/bool (that person was already ignoring the TValue!) will probably get a build-break since the returned tuple won't be bool anymore `Deconstruct(out bool isSuccess, out bool isFailed)` was moved from ResultBase to the Results class (the one without TValue) so it can still use that isSuccess/isFailed deconstruction.
Drizin
force-pushed
the
feature-resultOrErrors-deconstruct
branch
from
a07f12e
to
5b300e6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently all deconstructors return a pair
bool isSuccess/bool isFailed, which is slightly redundant (IMO only one would be enough, and if we are just interested in a single bool result we can just get the prop instead of using a desconstructor).It becomes even more redundant in
Result<TValue>since in that case we're also (we should be!) returningTValue valueand possiblyList<IError> errors- success/failure can just be infered from those values (if they are null) instead of having 4 different methods for testing the same thing.Previously I've been just discarding the redundant variables:
var (_, _, result, errors) = MyServiceCall(),but I thought that this cleaner/concise syntax could benefit everyone - so the 2-tuple deconstruct would just return us
TValue valueandList<IError> errors. (This concise style is used in golang and becoming popular elsewhere):var (result, errors) = MyServiceCall().This PR adds to Result that new golang-style deconstruct:
Deconstruct(out TValue value, out List<IError> errors).ResultBase still has
Deconstruct(out bool isSuccess, out bool isFailed), so non-generic Results (without TValue) can still use that isSuccess/isFailed deconstruction (which IMO is redundant and provide little value).Breaking change: if someone uses
Result<TValue>and do the 2-tuple deconstruction (which is unlikely since it would ignore the possible return value and also the error list) then this deconstruction will break (will get different types and most likely will break compilation).The
<bool,bool>deconstructor (which I think should be removed) was moved down fromResultBasetoResult. So that 2-tuple deconstruct is still available when there's no underlyingTValue(people usingResult<TValue>should never be using that construct in the first place since it ignores the important TValue).