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

Fixer: Prefer Memory overloads for Stream async Read/Write methods #3592

Merged

Conversation

carlossanlop
Copy link
Member

Fixes dotnet/runtime#33790

Summary

If the user invokes any of these methods from an object that is or inherits from Stream:

ReadAsync(byte[] buffer, int offset, int count)
ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
WriteAsync(byte[] buffer, int offset, int count)
WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)

We should suggest to instead use:

ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken)
WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken)

Works for both C# and VB.
All unit tests are passing locally.

@carlossanlop carlossanlop force-pushed the PreferStreamAsyncMemoryOverloads branch from 8774840 to 4a0be1e Compare May 1, 2020 00:19
@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #3592 into master will decrease coverage by 0.00%.
The diff coverage is 98.21%.

@@            Coverage Diff             @@
##           master    #3592      +/-   ##
==========================================
- Coverage   95.15%   95.15%   -0.01%     
==========================================
  Files        1069     1070       +1     
  Lines      242339   242302      -37     
  Branches    15753    15765      +12     
==========================================
- Hits       230596   230553      -43     
- Misses      10000    10002       +2     
- Partials     1743     1747       +4     

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #3592 into master will increase coverage by 0.00%.
The diff coverage is 97.32%.

@@           Coverage Diff            @@
##           master    #3592    +/-   ##
========================================
  Coverage   95.15%   95.16%            
========================================
  Files        1069     1071     +2     
  Lines      242339   243061   +722     
  Branches    15753    15789    +36     
========================================
+ Hits       230596   231305   +709     
- Misses      10000    10003     +3     
- Partials     1743     1753    +10     

@carlossanlop
Copy link
Member Author

I also need to resolve @mavasani 's request to group the description and title messages into one: #3586

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

Overall, LGTM. Thanks for extensive tests. I should be good to sign off once the last set of suggestions are addressed.

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM. I have couple more suggestions:

  1. Use new MyCodeAction as MyCodeAction.Create will do the same thing as CodeAction.Create
  2. Use localized title string without method name as the first argument for title.

@carlossanlop
Copy link
Member Author

LGTM. I have couple more suggestions:

  1. Use new MyCodeAction as MyCodeAction.Create will do the same thing as CodeAction.Create
  2. Use localized title string without method name as the first argument for title.

Fixed, @mavasani. Thanks. I'll merge as soon as the build finishes.

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.

Prefer Memory<T> overloads for Stream.Read/WriteAsync
4 participants