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

Stop retaining self in rxReuestWithProgress #1311

Merged
merged 2 commits into from Oct 2, 2017

Conversation

Projects
None yet
6 participants
@AndrewSB
Member

AndrewSB commented Sep 26, 2017

Fixes inconsitency in #1294

@AndrewSB AndrewSB changed the base branch from master to 10.0.0-dev Sep 26, 2017

@MoyaBot

This comment has been minimized.

Show comment
Hide comment
@MoyaBot

MoyaBot Sep 26, 2017

1 Warning
⚠️ The library files were changed, but the tests remained unmodified. Consider updating or adding to the tests to match the library changes.

Generated by 🚫 Danger

MoyaBot commented Sep 26, 2017

1 Warning
⚠️ The library files were changed, but the tests remained unmodified. Consider updating or adding to the tests to match the library changes.

Generated by 🚫 Danger

@SD10

This comment has been minimized.

Show comment
Hide comment
@SD10

SD10 Sep 26, 2017

Member

@AndrewSB I grabbed the error from the build log so you don't need to go hunting 😃
/Users/distiller/Moya/Sources/RxMoya/MoyaProvider+Rx.swift:64:17: value of optional type 'Cancellable?' not unwrapped; did you mean to use '!' or '?'?

Also, we have started to formalize the CHANGELOG guidelines. I noticed you added this under the Changed section but it could be better under Fixed. Assuming this behavior was incorrect. There's a lot of discussion going on in the other thread and I got lost.

Member

SD10 commented Sep 26, 2017

@AndrewSB I grabbed the error from the build log so you don't need to go hunting 😃
/Users/distiller/Moya/Sources/RxMoya/MoyaProvider+Rx.swift:64:17: value of optional type 'Cancellable?' not unwrapped; did you mean to use '!' or '?'?

Also, we have started to formalize the CHANGELOG guidelines. I noticed you added this under the Changed section but it could be better under Fixed. Assuming this behavior was incorrect. There's a lot of discussion going on in the other thread and I got lost.

@sunshinejr

Thanks for doing this, @AndrewSB! Just two comments from me. (Seems like I duplicated @SD10, didn't see it before reviewing 😓)

Show outdated Hide outdated Changelog.md Outdated
let response: Observable<ProgressResponse> = Observable.create { observer in
let cancellableToken = self.request(token, callbackQueue: callbackQueue, progress: progressBlock(observer)) { result in
let response: Observable<ProgressResponse> = Observable.create { [weak self] observer in
let cancellableToken = self?.request(token, callbackQueue: callbackQueue, progress: progressBlock(observer)) { result in

This comment has been minimized.

@sunshinejr

sunshinejr Sep 26, 2017

Member

You forgot to add ? in Dispsables.create { cancellableToken.cancel } below 😄

@sunshinejr

sunshinejr Sep 26, 2017

Member

You forgot to add ? in Dispsables.create { cancellableToken.cancel } below 😄

@volodg

This comment has been minimized.

Show comment
Hide comment
@volodg

volodg Sep 26, 2017

This solution contains a bug, example:

    var stream = Observable<Void>.create { observer in
      return Disposables.create {
      }
    }
    
    stream.subscribe(onError: { _ in
      print("doOnError")
    }, onCompleted: {
      print("doOnCompleted")
    }, onDispose: {
      print("doOnDispose")
    })

No callback is called !!! Observable should not be silent. If you are saying it is disposed when self is nil, call onDispose callback

volodg commented Sep 26, 2017

This solution contains a bug, example:

    var stream = Observable<Void>.create { observer in
      return Disposables.create {
      }
    }
    
    stream.subscribe(onError: { _ in
      print("doOnError")
    }, onCompleted: {
      print("doOnCompleted")
    }, onDispose: {
      print("doOnDispose")
    })

No callback is called !!! Observable should not be silent. If you are saying it is disposed when self is nil, call onDispose callback

@volodg

onDispose callback not called in case of self is nil

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Sep 26, 2017

Member

Sorry about the sloppy pull requests @SD10 @sunshinejr, both here and #1292. I'll work on testing my changes before submitting them so you guys don't have to play compiler for me -- I've been doing less Swift lately so it looks like my intuition is less on point than it used to be 😥

Member

AndrewSB commented Sep 26, 2017

Sorry about the sloppy pull requests @SD10 @sunshinejr, both here and #1292. I'll work on testing my changes before submitting them so you guys don't have to play compiler for me -- I've been doing less Swift lately so it looks like my intuition is less on point than it used to be 😥

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 26, 2017

Member

No worries @AndrewSB! I see that you have less time so it is really appreciated that you still contribute, thank you for that 👏

Member

sunshinejr commented Sep 26, 2017

No worries @AndrewSB! I see that you have less time so it is really appreciated that you still contribute, thank you for that 👏

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 26, 2017

Codecov Report

Merging #1311 into 10.0.0-dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           10.0.0-dev    #1311   +/-   ##
===========================================
  Coverage       87.09%   87.09%           
===========================================
  Files              22       22           
  Lines             736      736           
===========================================
  Hits              641      641           
  Misses             95       95
Impacted Files Coverage Δ
Sources/RxMoya/MoyaProvider+Rx.swift 91.66% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 059979c...748c8d7. Read the comment docs.

codecov-io commented Sep 26, 2017

Codecov Report

Merging #1311 into 10.0.0-dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           10.0.0-dev    #1311   +/-   ##
===========================================
  Coverage       87.09%   87.09%           
===========================================
  Files              22       22           
  Lines             736      736           
===========================================
  Hits              641      641           
  Misses             95       95
Impacted Files Coverage Δ
Sources/RxMoya/MoyaProvider+Rx.swift 91.66% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 059979c...748c8d7. Read the comment docs.

@volodg

This comment has been minimized.

Show comment
Hide comment
@volodg

volodg Sep 29, 2017

@AndrewSB
this example:

    let singleStream = Observable<Int>
      .interval(1, scheduler: MainScheduler.instance)
      .timeout(1, scheduler: MainScheduler.instance)
      .asSingle()
      .do(onNext: { _ in
      print("onNext")
    }, onError: { _ in
      print("onError")
    }, onSubscribe: {
      print("onSubscribe")
    }, onSubscribed: {
      print("onSubscribed")
    }, onDispose: {
      print("onDispose")
    })
    
    singleStream.subscribe().dispose()

prints:

onSubscribe
onSubscribed
onDispose

So onDispose at least should be called, please fix

volodg commented Sep 29, 2017

@AndrewSB
this example:

    let singleStream = Observable<Int>
      .interval(1, scheduler: MainScheduler.instance)
      .timeout(1, scheduler: MainScheduler.instance)
      .asSingle()
      .do(onNext: { _ in
      print("onNext")
    }, onError: { _ in
      print("onError")
    }, onSubscribe: {
      print("onSubscribe")
    }, onSubscribed: {
      print("onSubscribed")
    }, onDispose: {
      print("onDispose")
    })
    
    singleStream.subscribe().dispose()

prints:

onSubscribe
onSubscribed
onDispose

So onDispose at least should be called, please fix

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 29, 2017

Member

@volodg this PR focus rather on inconsistency than on fixing the Single behavior - let's focus at one thing at a time. @AndrewSB I think there is only a Changelog fix left to merge 👍

Member

sunshinejr commented Sep 29, 2017

@volodg this PR focus rather on inconsistency than on fixing the Single behavior - let's focus at one thing at a time. @AndrewSB I think there is only a Changelog fix left to merge 👍

@AndrewSB

This comment has been minimized.

Show comment
Hide comment
@AndrewSB

AndrewSB Sep 29, 2017

Member

@volodg from my understanding, onDispose is already being called if the Single is disposed. in this comment #1294 (comment) for example, the .debug() operator prints isDisposed.

@sunshinejr sounds good! Once I commit can you re-review before merge?

Member

AndrewSB commented Sep 29, 2017

@volodg from my understanding, onDispose is already being called if the Single is disposed. in this comment #1294 (comment) for example, the .debug() operator prints isDisposed.

@sunshinejr sounds good! Once I commit can you re-review before merge?

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 29, 2017

Member

Sure thing 👍

Member

sunshinejr commented Sep 29, 2017

Sure thing 👍

@AndrewSB AndrewSB requested a review from SD10 Sep 29, 2017

re-review

@AndrewSB AndrewSB requested a review from sunshinejr Sep 29, 2017

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Sep 30, 2017

Member

@AndrewSB I don't see any changes to the Changelog 🤔 What I'm talking about is that comment about making it in the separate column Fixed, instead of having it in Changed.

Member

sunshinejr commented Sep 30, 2017

@AndrewSB I don't see any changes to the Changelog 🤔 What I'm talking about is that comment about making it in the separate column Fixed, instead of having it in Changed.

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 2, 2017

Member

I went ahead and fixed the Changelog. It should be good to go now. Please re-review @Moya/contributors.

Member

sunshinejr commented Oct 2, 2017

I went ahead and fixed the Changelog. It should be good to go now. Please re-review @Moya/contributors.

@SD10

SD10 approved these changes Oct 2, 2017

Looks good to me. I was just waiting on the CHANGELOG and didn't want to dupe your requests

@sunshinejr

Thanks again @AndrewSB 🎉

@sunshinejr

This comment has been minimized.

Show comment
Hide comment
@sunshinejr

sunshinejr Oct 2, 2017

Member

@SD10 thank you!

Member

sunshinejr commented Oct 2, 2017

@SD10 thank you!

@sunshinejr sunshinejr merged commit fa8e6de into 10.0.0-dev Oct 2, 2017

1 check was pending

ci/circleci Your tests are queued behind your running builds
Details

@sunshinejr sunshinejr deleted the dont-retain-yo branch Oct 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment