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

Use Array.Empty<T>() #900

Merged
merged 5 commits into from
Jan 18, 2023
Merged

Use Array.Empty<T>() #900

merged 5 commits into from
Jan 18, 2023

Conversation

martincostello
Copy link
Contributor

Changes

Use Array.Empty<string>() instead of allocating a new array.

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #

Use `Array.Empty<string>()` instead of allocating a new array.
@martincostello martincostello requested a review from a team January 17, 2023 18:51
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Merging #900 (903b53d) into main (6e38cfd) will decrease coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 903b53d differs from pull request most recent head fba5626. Consider uploading reports for the commit fba5626 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #900      +/-   ##
==========================================
- Coverage   68.55%   68.44%   -0.12%     
==========================================
  Files         183      182       -1     
  Lines        6981     6952      -29     
==========================================
- Hits         4786     4758      -28     
+ Misses       2195     2194       -1     
Impacted Files Coverage Δ
...ntation.AWSLambda/Implementation/AWSLambdaUtils.cs 88.05% <100.00%> (ø)
...ensions.AzureMonitor/ApplicationInsightsSampler.cs

@Oberon00
Copy link
Member

This looks like something that could & should be checked by a linter BTW. Any rules we could enable to catch this automatically?

@martincostello
Copy link
Contributor Author

There's a built-in rule for this (I forget the number) in fairly recent versions of the .NET SDK (6+ at latest) that should detect such usage if enabled.

@Kielek
Copy link
Contributor

Kielek commented Jan 18, 2023

I suppose that it can be detected if you enable <AnalysisLevel>latest-All</AnalysisLevel> in csproj. See for main repository: open-telemetry/opentelemetry-dotnet#3958.

The warning: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1825

@martincostello
Copy link
Contributor Author

I'd be happy to do a PR to enable that separately - I think otherwise turning it on might find other unrelated diagnostics that need fixing and quickly make the scope of this otherwise small PR spiral.

@cijothomas cijothomas merged commit b53b6b9 into open-telemetry:main Jan 18, 2023
@martincostello martincostello deleted the array-empty branch January 18, 2023 20:34
@martincostello martincostello mentioned this pull request Jan 19, 2023
2 tasks
@martincostello
Copy link
Contributor Author

I suppose that it can be detected if you enable <AnalysisLevel>latest-All</AnalysisLevel> in csproj. See for main repository: open-telemetry/opentelemetry-dotnet#3958.

The warning: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1825

Have opened #908 to enable analysers.

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.

4 participants