Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten error by removing ActionError #158

Closed
wants to merge 49 commits into from

Conversation

dangthaison91
Copy link
Contributor

I really sorry for a long time not active. This replaces PR for #138 and will resolve #119 (original by @bobgodwinx )

After referring Action's implementation from ReactiveSwift, I decide to add disabledErrors: Observable<Error> and remove ActionError. This will make breaking change but will help end users avoid misunderstand between .errors/.underlyingErrors and .executionErrors from the original proposal. I add ActionDisabledError for better debugging.

What do you think @bobgodwinx

XavierDK and others added 30 commits November 27, 2017 21:20
[Fix][Carthage] Remove Submodules file
…tings

Update project settings to silence warning.
Fix build failure on New Build System (default on Xcode 10)
@ashfurrow
Copy link
Member

Makes sense – combined with your work on #159, do you think we should target this for the 4.0 branch and release?

@dangthaison91
Copy link
Contributor Author

@ashfurrow Yeah, this is breaking changes, I will have to add changelog for this before merging it in 4.0.

@ashfurrow
Copy link
Member

Makes sense – should we update the 4.0 branch, and then change this PR to point to that branch?

@dangthaison91 dangthaison91 changed the base branch from master to 4.0 October 16, 2018 15:09
@dangthaison91 dangthaison91 changed the title Flatten error by removing ActionError [WIP] Flatten error by removing ActionError Oct 16, 2018
@dangthaison91
Copy link
Contributor Author

This PR requires the PR #160 to be merged first

@ashfurrow
Copy link
Member

Cool. Once this is merged, and 4.0 branch looks good, let's plan to release 4.0. /cc @freak4pc

@dangthaison91 dangthaison91 changed the title [WIP] Flatten error by removing ActionError Flatten error by removing ActionError Oct 17, 2018
@dangthaison91
Copy link
Contributor Author

Should I merge this @ashfurrow ?

@freak4pc
Copy link
Member

Hey @dangthaison91 I think you wrongly created this PR with a weird rebase - look at the commits, there are 42 including other people's not just yours.

Please don't merge without a review, since this repo is actually commonly used by people it's critical people will be able to review any breaking changes such as these.

@dangthaison91
Copy link
Contributor Author

Hmm, should I close this PR and create another one @freak4pc ?

@ashfurrow
Copy link
Member

@dangthaison91 that sounds like a good idea – let me know if you run into git trouble.

@freak4pc
Copy link
Member

freak4pc commented Oct 22, 2018 via email

@dangthaison91 dangthaison91 deleted the flatten-error branch October 23, 2018 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants