Skip to content

Update method names for client registrations#2630

Merged
sebastienros merged 3 commits intomainfrom
sebros/clients
Mar 7, 2024
Merged

Update method names for client registrations#2630
sebastienros merged 3 commits intomainfrom
sebros/clients

Conversation

@sebastienros
Copy link
Copy Markdown
Contributor

@sebastienros sebastienros commented Mar 5, 2024

Fixes #1510

We may consider different names based on taste. Sometimes the name matches the client type, and sometimes not.

Examples:

  • AddRabbitMQClient(), maybe remove the MQ. The client name is IConnection.
  • AddRedisClient(), the client name is IConnectionMultiplexer, I think it's ok.
  • AddCosmosClient(), the client name is CosmosClient but might sound better with AddCosmosDBClient.
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-integrations Issues pertaining to Aspire Integrations packages label Mar 5, 2024
@sebastienros sebastienros requested a review from mitchdenny March 5, 2024 00:36
@davidfowl
Copy link
Copy Markdown
Contributor

Do we want to add new ones and deprecate the old ones?

@sebastienros
Copy link
Copy Markdown
Contributor Author

Do we want to add new ones and deprecate the old ones?

So ship a preview (at least) with deprecations, to provide a hint about how to update existing apps? I like it.

@davidfowl
Copy link
Copy Markdown
Contributor

So ship a preview (at least) with deprecations, to provide a hint about how to update existing apps? I like it.

Yep.

@eerhardt
Copy link
Copy Markdown
Member

eerhardt commented Mar 5, 2024

So ship a preview (at least) with deprecations, to provide a hint about how to update existing apps?

Make sure we log an issue in the correct milestone to remove these deprecated methods. We don't want to ship GA with them.

@davidfowl
Copy link
Copy Markdown
Contributor

#2417

@sebastienros
Copy link
Copy Markdown
Contributor Author

I pushed a brand new set of changes with the feedback I got. Now it keeps the same prefixes as before with the added "Client" suffix when necessary. And the existing one is marked as [Obsolete].

Comment on lines +21 to +23
builder.AddAzureBlobServiceClient("blob");
builder.AddAzureTableServiceClient("table");
builder.AddAzureQueueServiceClient("queue");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do folks think about shortening these?

Suggested change
builder.AddAzureBlobServiceClient("blob");
builder.AddAzureTableServiceClient("table");
builder.AddAzureQueueServiceClient("queue");
builder.AddAzureBlobsClient("blob");
builder.AddAzureTablesClient("table");
builder.AddAzureQueuesClient("queue");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The underlying class names are:

  • Azure.Storage.Blobs.BlobServiceClient
  • Azure.Data.Tables.TableServiceClient
  • Azure.Storage.Queues.QueueServiceClient

Is it confusing if we drop "Service" from these names? In general I like to stick with the naming of the underlying thing. But agree that @DamianEdwards's suggestion looks cleaner.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keep the underlying names, that way we don't need to think too much 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW we're adding Client to all the methods which happens to match the underlying types in this case but not all cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I won’t die on this hill but we should optimize for less case by case naming conventions.

I could go with either.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should aim to go with something generic like Add[LogicalName]Client, e.g. AddRedisClient, AddAzureBlobsClient, AddSqlClient, etc. (obvious exception for EF Core related methods that operate on a DbContext).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets close then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Lets close then.

Is that "Just do it"?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yea

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, also changed AddAzureKeyVaultSecretsClient

# Conflicts:
#	playground/cdk/CdkSample.ApiService/Program.cs
@sebastienros sebastienros merged commit 6c7788d into main Mar 7, 2024
@sebastienros sebastienros deleted the sebros/clients branch March 7, 2024 01:07
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-integrations Issues pertaining to Aspire Integrations packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider renaming component or resource methods to disambiguate and reduce confusion

4 participants