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

Add analyzers/fixers for Environment.ProcessPath and CurrentManagedThreadId #4909

Merged

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Mar 1, 2021

@stephentoub stephentoub requested a review from a team as a code owner March 1, 2021 04:09
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #4909 (d45a6f6) into release/6.0.1xx-preview3 (c054652) will decrease coverage by 0.00%.
The diff coverage is 98.09%.

@@                     Coverage Diff                      @@
##           release/6.0.1xx-preview3    #4909      +/-   ##
============================================================
- Coverage                     95.57%   95.56%   -0.01%     
============================================================
  Files                          1175     1175              
  Lines                        269054   269300     +246     
  Branches                      16302    16314      +12     
============================================================
+ Hits                         257144   257363     +219     
- Misses                         9784     9805      +21     
- Partials                       2126     2132       +6     

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

I think this should target .NET 6 branch (6.0.1xx-preview3).

@jmarolf
Copy link
Contributor

jmarolf commented Mar 1, 2021

@stephentoub as @Youssef1313 says, master is currently being used to service nuget packages. This PR should target https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx-preview3

@stephentoub stephentoub changed the base branch from master to release/6.0.1xx-preview3 March 1, 2021 17:29
@stephentoub
Copy link
Member Author

Feedback addressed and rebased on release/6.0.1xx-preview3

@Youssef1313
Copy link
Member

Test failure is unrelated and was fixed in dotnet/roslyn#51244

@"
using System;
using System.Diagnostics;
using System.Threading;
Copy link
Member

Choose a reason for hiding this comment

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

The codefix could cleanup the now-unused usings. This is not necessary but probably good to have as a follow-up. Should I open an issue tracking this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we do that with other analyzers? Removing usings seems orthogonal and a style preference, not something we should be doing automatically just because we happened to remove the last use of something in that namespace. We could actually be creating more work and confusion for the developer.

@stephentoub
Copy link
Member Author

@mavasani, any feedback before it's merged? Thanks.

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.

Looks great to me!

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.

Add analyzer for Environment.CurrentManagedThreadId Add analyzer for Environment.ProcessPath
5 participants