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 consistent pattern for namespace of extension methods #1576

Merged

Conversation

alanwest
Copy link
Member

This is an alternative to #1541.

I'm beginning to lean more towards leaving things alone for the most part. That is, I think the general rule of thumb that has been followed is to place extension methods in the same namespace as the type that they are extending. This is what Microsoft has followed with many of their libraries (e.g., Microsoft.Extensions.Configuration, Microsoft.Extensions.Logging, etc.).

However, there are a few places that do not follow this rule. I believe some of them are intentional exceptions to the rule, but some of them may have been unintentional.

I this PR, I've tracked down all of the public extension methods that are not contained in the same namespace as the type they are extending and changed them to be in the same namespace. There were two notable exceptions that I think are sensible exceptions to this general rule - I added a comment explaining why - please confirm the accuracy of my comments.

Questions:

  • Does this seem like a good pattern to follow?
  • Are there any namespace changes in this PR that anyone feels should be an exception to the pattern?

@alanwest alanwest added question Further information is requested pr:do-not-merge labels Nov 18, 2020
@alanwest alanwest requested a review from a team November 18, 2020 01:55
@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #1576 (a8967ce) into master (5995fd9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1576   +/-   ##
=======================================
  Coverage   80.73%   80.73%           
=======================================
  Files         242      242           
  Lines        6544     6544           
=======================================
  Hits         5283     5283           
  Misses       1261     1261           
Impacted Files Coverage Δ
src/OpenTelemetry.Api/ActivityContextExtensions.cs 100.00% <ø> (ø)
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 94.11% <ø> (ø)
...porter.Console/ConsoleExporterLoggingExtensions.cs 0.00% <ø> (ø)
...orter.InMemory/InMemoryExporterHelperExtensions.cs 0.00% <ø> (ø)
...rter.InMemory/InMemoryExporterLoggingExtensions.cs 0.00% <ø> (ø)
...ter.Prometheus/PrometheusRouteBuilderExtensions.cs 100.00% <ø> (ø)
...enTelemetry/Resources/ResourceBuilderExtensions.cs 95.65% <ø> (ø)

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

This looks good to me. We want to add this to changelog before merge.

@cijothomas
Copy link
Member

@reyang @CodeBlanch please share your thoughts as well.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

…anwest/opentelemetry-dotnet into alanwest/consistent-extension-methods
@alanwest alanwest removed pr:do-not-merge question Further information is requested labels Nov 18, 2020
@alanwest alanwest changed the title Question: Use consistent pattern for namespace of extension methods? Use consistent pattern for namespace of extension methods Nov 18, 2020
@cijothomas cijothomas merged commit b710c31 into open-telemetry:master Nov 18, 2020
@CodeBlanch
Copy link
Member

I definitely agree with the approach that (in most cases) extensions should live in the namespace of whatever is being extended. Thanks @alanwest!

@alanwest alanwest deleted the alanwest/consistent-extension-methods branch March 9, 2021 18:47
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.

5 participants