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

Don't import System when it's already imported by global using #6358

Merged
merged 6 commits into from
Dec 21, 2022

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 15, 2022

Fixes #5652

@Youssef1313 Youssef1313 marked this pull request as ready for review December 15, 2022 14:30
@Youssef1313 Youssef1313 requested a review from a team as a code owner December 15, 2022 14:30
@@ -30,12 +30,11 @@ namespace Microsoft.NetCore.Analyzers.Runtime
/// </summary>
public abstract class PreferStreamAsyncMemoryOverloadsFixer : CodeFixProvider
{
private static readonly SyntaxAnnotation s_asMemorySymbolAnnotation = new("SymbolId", "System.MemoryExtensions");
Copy link
Member Author

Choose a reason for hiding this comment

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

This is quite an implementation detail on Roslyn. I have an API proposal to expose it in cleaner way. dotnet/roslyn#65041

@Youssef1313
Copy link
Member Author

@CyrusNajmabadi Looks like I got a repro here for the line ending issue. dotnet/roslyn#65081

Can you please take a look? Thanks!

@Youssef1313
Copy link
Member Author

My bad. This is an extra line, not line ending difference. This is completely unrelated to dotnet/roslyn#65081.

@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Merging #6358 (fde7601) into main (9e92aa9) will increase coverage by 0.02%.
The diff coverage is 94.87%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6358      +/-   ##
==========================================
+ Coverage   96.08%   96.11%   +0.02%     
==========================================
  Files        1360     1361       +1     
  Lines      315522   316072     +550     
  Branches    10187    10187              
==========================================
+ Hits       303173   303777     +604     
+ Misses       9917     9865      -52     
+ Partials     2432     2430       -2     

@Youssef1313
Copy link
Member Author

and now ubuntu only failure related to end of lines 😄
So it is dotnet/roslyn#65081

@Youssef1313
Copy link
Member Author

@mavasani Failure here is a Roslyn bug that we have in other codefixes that uses AddImportsAnnotation.

Is it okay to disable the test on Linux? Any workaround you can think of?

@mavasani
Copy link
Contributor

Is it okay to disable the test on Linux?

Yes, that sounds reasonable. I believe we have an attribute to execute tests only on Windows platform that is used by some tests. I can't recall if it exists only in Roslyn or also in this repo.

@Youssef1313
Copy link
Member Author

Is it okay to disable the test on Linux?

Yes, that sounds reasonable. I believe we have an attribute to execute tests only on Windows platform that is used by some tests. I can't recall if it exists only in Roslyn or also in this repo.

It exists here and removal is tracked by #4586.

@mavasani mavasani merged commit f983f0e into dotnet:main Dec 21, 2022
@github-actions github-actions bot added this to the vNext milestone Dec 21, 2022
@Youssef1313 Youssef1313 deleted the using-system branch December 21, 2022 12:35
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.

CA1845: Adds unnecessary using statement when implicit usings exist
3 participants