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

remove unreachable ?? new ConsoleExporterOptions() in ConsoleExporter #6079

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SimonCropp
Copy link
Contributor

Fixes #
Design discussion issue #

Changes

since it cant be null

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@SimonCropp SimonCropp requested a review from a team as a code owner January 20, 2025 12:06
@github-actions github-actions bot added the pkg:OpenTelemetry.Exporter.Console Issues related to OpenTelemetry.Exporter.Console NuGet package label Jan 20, 2025
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.40%. Comparing base (b508b84) to head (d59c913).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6079      +/-   ##
==========================================
- Coverage   86.40%   86.40%   -0.01%     
==========================================
  Files         257      257              
  Lines       11649    11648       -1     
==========================================
- Hits        10065    10064       -1     
  Misses       1584     1584              
Files with missing lines Coverage Δ
.../OpenTelemetry.Exporter.Console/ConsoleExporter.cs 63.63% <ø> (-3.04%) ⬇️

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

I am fine with this change, but there is one comment to this.
Historically, this project was released without Nullability check. When introduced, this kind of checks stays in place to avoid any hidden breaking changes. What is more, it is still possible to pass null here by null forgiving operator !/disabling nullability in the project. IMO the problem is similar to the Debug.Assert discussed in #6101

@@ -10,7 +10,7 @@ public abstract class ConsoleExporter<T> : BaseExporter<T>

protected ConsoleExporter(ConsoleExporterOptions options)
{
this.options = options ?? new ConsoleExporterOptions();
this.options = options;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the correct direction.

This is a public API. We can't control what users will pass it. Even though we say it shouldn't be null (via the nullable annotations), it is still possible for users to pass null.

It probably should have been written like this:

    protected ConsoleExporter(ConsoleExporterOptions options)
    {
       Guard.ThrowIfNull(options);

       this.options = options;
    }

But technically a breaking change to do now.

If we really want to clean up the oddity, why not just embrace the fact that it allows null?

-   protected ConsoleExporter(ConsoleExporterOptions options)
+   protected ConsoleExporter(ConsoleExporterOptions? options)
    {
        this.options = options ?? new ConsoleExporterOptions();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.Console Issues related to OpenTelemetry.Exporter.Console NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants