Skip to content
This repository has been archived by the owner on Jul 3, 2022. It is now read-only.

Replace custom Box and Result with microframeworks #49

Closed
wants to merge 16 commits into from

Conversation

nvh
Copy link
Contributor

@nvh nvh commented May 12, 2015

I went ahead and implemented the changes discussed in #48. The basic idea is: replace the Result type completely, but keep as much of the old functionality as possible.

Because of framework limitations, it's not possible to publicly extend Result once it is in a library. Luckily the majority of the functionality is implemented by antitypical/Result as well.

All tests succeed, but I disabled a few, I will comment om them individually.

@@ -151,26 +161,26 @@ class ResultTests: XCTestCase {
XCTAssert(r.isFailure)
XCTAssertEqual(r.error!.domain, "DivisionByZeroError")
}

func testRecoverNeeded() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recover functionality is only implemented in antitypical/Result by the ?? operator. It's not possible to add recover as such, but we could just replace recover() with ?? here

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking into this as well and I think this is the main difference between BrightFutures' Result and antitypical/Result. We could try to file a PR for antitypical/Result with the recover and recoverWith functions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Thomvis
Copy link
Owner

Thomvis commented May 12, 2015

Thanks for the PR!

@Thomvis Thomvis closed this May 12, 2015
@Thomvis Thomvis reopened this May 12, 2015
@Thomvis
Copy link
Owner

Thomvis commented May 12, 2015

Oops, sorry about that. (The closing and opening.)

I'll respond to your comments individually, but I think the way forward is to see if we can get some of the missing features implemented in https://github.com/antitypical/Result and then (either with or without the changes in antitypical/Result) work towards a BrightFutures 2.0 that has this as the main change. I think we can still support 1.x for the time being as well.

We might want to consider making Future<T> a Future<T,Error>, but I think I am okay with not doing that at the same time.

@robrix
Copy link

robrix commented May 12, 2015

Please don’t hesitate to let me know if there’s anything Result or Box could be doing to make this transition easier!

@nvh
Copy link
Contributor Author

nvh commented May 13, 2015

I think the way forward is to see if we can get some of the missing features implemented in https://github.com/antitypical/Result and then (either with or without the changes in antitypical/Result) work towards a BrightFutures 2.0 that has this as the main change.

Sounds great!

We might want to consider making Future a Future<T,Error>, but I think I am okay with not doing that at the same time.

I think it would be best to decide now if we want to create Future<T,Error> and if we decide to, make that change now, and not quickly follow up 2.0 with 3.0 to add that change. It doesn't have to be part of this pull request, though.

nvh added 3 commits May 13, 2015 09:51
made `flatMap() -> Future<U>` a free function instead of a instance of `Result`
@robrix
Copy link

robrix commented May 13, 2015

@nvh
Copy link
Contributor Author

nvh commented May 13, 2015

I've added all discussed changes, so I'm all for merging this PR now

@Thomvis Thomvis added this to the 2.0 milestone May 14, 2015
@Thomvis
Copy link
Owner

Thomvis commented May 14, 2015

I have merged this PR into the 2.0-development branch. PR #51 tracks the progress towards BrightFutures 2.0.

Thanks again!

@Thomvis Thomvis closed this May 14, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants