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

Current dotnet/extensions API Surface #4373

Closed
wants to merge 14 commits into from
Closed

Conversation

geeknoid
Copy link
Member

@geeknoid geeknoid commented Sep 7, 2023

Please review to make sure you're happy with:

  • Package names
  • Namespace names
  • Type names
  • Method names

Last chance to speak up before November is upon us.

Microsoft Reviewers: Open in CodeFlow

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Gone through the NoDocs

@geeknoid
Copy link
Member Author

geeknoid commented Sep 8, 2023

@danmoseley Please don't push commits to this PR. This is all auto-generated stuff, so any edits will be lost.

@danmoseley
Copy link
Member

danmoseley commented Oct 2, 2023

@geeknoid is all feedback addressed -- or more to the point, this API dump is now out of date with changes you've made?

maybe we should close this now?

edit: oh, Jose pointed out that you're keeping this up to date.

{
public static IServiceCollection AddFakeRedaction(this IServiceCollection services);
public static IServiceCollection AddFakeRedaction(this IServiceCollection services, Action<FakeRedactorOptions> configure);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is it missing public static IServiceCollection AddFakeRedaction(this IServiceCollection services, IConfigurationSection section);

Comment on lines +11 to +18
public sealed class CounterAttribute : Attribute
{
public string? Name { get; set; }
public string[]? TagNames { get; }
public Type? Type { get; }
public CounterAttribute(params string[] tagNames);
public CounterAttribute(Type type);
}
Copy link
Member

@JamesNK JamesNK Oct 10, 2023

Choose a reason for hiding this comment

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

I'm not a fan of the shape of CounterAttribute and friends. I could imagine someone entering [Counter("my.cool.counter.name")] and expecting a counter with that name but instead accidentally calling the CounterAttribute(params string[] tagNames) ctor and getting a counter with an inferred name and tag named my.cool.counter.name instead. Feels like pit of failure.

The most significant piece of information for a counter is its name. It should be in the constructor. Also, it should always be required. Infering it from a return type doesn't feel good. Thought should be put into the name.

Also, the Type property name isn't descriptive. TagsType is still a little confusing because tags can have a type but I can't think of a better name.

Suggested change
public sealed class CounterAttribute : Attribute
{
public string? Name { get; set; }
public string[]? TagNames { get; }
public Type? Type { get; }
public CounterAttribute(params string[] tagNames);
public CounterAttribute(Type type);
}
public sealed class CounterAttribute : Attribute
{
public string? Name { get; }
public string[]? TagNames { get; set; }
public Type? TagsType { get; set; }
public CounterAttribute(string name);
}

Usage:

[Counter("my.cool.counter.name", TagNames = [ "one", "two", "etc" ])]

If someone sets TagNames and TagType then throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@geeknoid @dariusclay @iliar-turdushev folks, any thoughts here?
Personally I see the point of the suggested change, but @JamesNK please bear in mind that by default the source-gen will take a type's name as an instrument name, that's why its API was made that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with the suggestion, but it will be a breaking change for existing customers, and complicate their migration to dotnet/extensions. Is it OK?

The only change I'd make to the suggestion is mark name argument as optional:

public CounterAttribute(string? name = null);

If name is null then we infer the name of a counter from the return type of a method.

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 comment could have been addressed a few weeks back, but now it's too late. We can no longer do breaking changes in dotnet/extensions. So let's improve this incrementally when possible, without breaking things.

using Microsoft.AspNetCore.Hosting.Server;
using Microsoft.Extensions.Hosting;

namespace Microsoft.AspNetCore.Testing;
Copy link
Member

Choose a reason for hiding this comment

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

These extensions should be in the same namespace as their extended types, IWebHostBuilder and IHost (even if it means splitting the class). I'll take care of that.

@geeknoid geeknoid closed this Oct 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2023
@RussKie RussKie deleted the geeknoid/api branch February 11, 2024 23:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.