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

Remove System.Linq.Async dependency #1976

Merged
merged 1 commit into from Mar 31, 2020
Merged

Remove System.Linq.Async dependency #1976

merged 1 commit into from Mar 31, 2020

Conversation

ob-stripe
Copy link
Contributor

r? @remi-stripe
cc @stripe/api-libraries

Remove the dependency on System.Linq.Async, by reimplementing the .ToEnumerable() method directly.

Fixes #1974.

@ob-stripe
Copy link
Contributor Author

@ob-stripe
Copy link
Contributor Author

Sorry for force-pushing mid review :( I moved most of the code into AsyncUtils.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for adding that remark on code borrowing — was just about to ask for something similar :)

@ob-stripe
Copy link
Contributor Author

Thanks Brandur!

@ob-stripe ob-stripe merged commit bbeb9d1 into master Mar 31, 2020
@ob-stripe ob-stripe deleted the ob-fix-1974 branch March 31, 2020 17:48
@ob-stripe
Copy link
Contributor Author

Released as 35.11.1.

@clement911
Copy link
Contributor

This code is a bit weird, they even mentioned it's an anti pattern.

Async over sync is often worse than just sync so another option would be to have the !NET45 ListRequestAutoPaging used the same implementation as the ListRequestAutoPaging used fo NET45 instead of doing sync over ListRequestAutoPagingAsync.. Then you could remove AsyncUtils altogether.

Just a suggestion...

@ob-stripe
Copy link
Contributor Author

ob-stripe commented Apr 2, 2020

Yeah, I was a little bit on the fence about it. The autopagination implementation is complex enough that I'd rather avoid duplicating the method, so I went with this.

(Of course right now the implementation is duplicated anyway because of NET45, but we will likely drop support for .NET Framework < 4.6.1 at some point.)

@clement911
Copy link
Contributor

Looks like it might cause deadlocks #1946 (comment)

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