Skip to content

Removed params overwrite#710

Merged
QuintinWillison merged 26 commits intomainfrom
705-push-listSubscriptions-ignore-params
Oct 19, 2021
Merged

Removed params overwrite#710
QuintinWillison merged 26 commits intomainfrom
705-push-listSubscriptions-ignore-params

Conversation

@martin-morek
Copy link
Copy Markdown
Contributor

@martin-morek martin-morek commented Sep 8, 2021

  • fixes #705
  • listSubscriptionsImpl() input parameters are not overwritten anymore

@QuintinWillison
Copy link
Copy Markdown
Contributor

Given @paddybyers originally worked on this code then I think it would be good to get his opinion on this change. There may be something we're missing.

@martin-morek
Copy link
Copy Markdown
Contributor Author

Test is failing. I'm trying to find out the expected behaviour in the issue ticket.

@uxduck
Copy link
Copy Markdown
Contributor

uxduck commented Sep 10, 2021

I mentioned this in a stand up with Paddy a few weeks ago and he confirmed this is not expected behaviour. The method is completely overwriting the params that a user sets, rendering the arguments a user selects completely useless. Please see #705 for more information.

@uxduck
Copy link
Copy Markdown
Contributor

uxduck commented Sep 14, 2021

I'd like to remove my approval for the time being, since I am not sure what's going here: it's now a draft PR and there has been some code changes.

@martin-morek
Copy link
Copy Markdown
Contributor Author

@ben-xd the status has been changed to draft since one test is always failing and it has to be investigated further. I reverted my code changes just to figurate out if those changes caused test failure (which seems not to be the case). Anyway I will change PR status to "Ready" as soon as test will pass.

@martin-morek martin-morek marked this pull request as ready for review October 18, 2021 17:30
Copy link
Copy Markdown
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

There is a lot of information in the linked issue which I was mostly able to follow when I read through it just now, but I'll admit it was a little overwhelming. It sounds like we're removing over-zealous behaviour from this SDK, matching more closely the implementation in the Cocoa SDK. That makes sense - and I'm always a fan of removing code. 😄

Thanks to @ikbalkaya for working on sorting out the emulator runtime issues. I do think there could still be an opportunity to document that better but let's work on that under another pull request.

@QuintinWillison QuintinWillison merged commit e9383fc into main Oct 19, 2021
@QuintinWillison QuintinWillison deleted the 705-push-listSubscriptions-ignore-params branch October 19, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

push.listSubscriptionsImpl method not respecting params

4 participants