search

OmerFlame / SwiftRant

devRant API library made in Swift.
GNU General Public License v3.0
3 stars 1 forks source link

use the Result type #8

Closed WilhelmOks closed 2 years ago

WilhelmOks commented 2 years ago

Currently, all of the return types are the tuple (String?, Output?). This has the issue that it's not immediately clear that the first one is the error message and the second one is the resulting value in case of success. Another issue is that there are technically possible values that should never occur. Specifically, (nil, nil) should never occur and also the error and result value both being not nil should never occur.

Swift has introduced the Result type exactly for this use case and I propose to adapt it instead of (String?, Output?). Specifically, it should be Result<Output, SwiftRantError> where SwiftRantError is a struct that conforms to Error and containts the non optional String message as the only stored variable.

This will require extensive and breaking changes, but I think it will result in more self explaining and robust code. The repeated doc text for the return values can be drastically reduced then because it only needs to explain the success case and only if the success type is not Void. Every other documentation will be implicitly encapsulated in the Return type.

I wanted to ask you first if you agree. Then one of us can start to implement the changes.

OmerFlame commented 2 years ago

I wholeheartedly agree, this is a stupid oversight I clearly glossed over from back when I was in the process of refactoring and rewriting the functions from the AltRant project into SwiftRant. This will be a lot of work but also a worthwhile quality-of-life improvement for the entire library. I want this to be one massive change though, not a gradual departure.

WilhelmOks commented 2 years ago

Allright. Let's agree to let each other know when one starts to implement this, so that we don't do it in parallel redundantly. We can write a comment here.

OmerFlame commented 2 years ago

I am starting work on it now!

OmerFlame commented 2 years ago

Believe it or not, I somehow focused so hard I already finished the job. You can see the new branch result_transition.

The only thing that remains before merging the branch is to figure out the documentation terminology for the new return types and completionHandles. Care to give a generic formula for it?

OmerFlame commented 2 years ago

I just opened a draft pull request #10, let's keep up the conversation from there.

OmerFlame commented 2 years ago

Marking as resolved.