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

Review use of ArgumentException/ArgumentNullException in all public APIs #2649

Closed
DamianEdwards opened this issue Mar 5, 2024 · 3 comments · Fixed by #6278
Closed

Review use of ArgumentException/ArgumentNullException in all public APIs #2649

DamianEdwards opened this issue Mar 5, 2024 · 3 comments · Fixed by #6278
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication good first issue Good for newcomers help wanted Issue that is a good candidate for community contribution.

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Mar 5, 2024

I noticed we seem to have lots of public APIs that accept reference type parameters not marked as nullable but then don't check that they're actually non-null and throw an ArgumentNullException when necessary. We should decide what we're doing and review all public APIs for appropriate argument validation.

e.g.

public static IResourceBuilder<T> WithEnvironment<T>(this IResourceBuilder<T> builder, string name, IResourceBuilder<ParameterResource> parameter) where T : IResourceWithEnvironment
{
return builder.WithEnvironment(context =>
{
context.EnvironmentVariables[name] = parameter.Resource;
});
}

@DamianEdwards DamianEdwards added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed area-meta untriaged labels Mar 5, 2024
@mitchdenny mitchdenny added this to the 8.1 milestone Apr 9, 2024
@mitchdenny
Copy link
Member

@JamesNK your PR mentioned this issue, do you think we've got 100% coverage here now from your #2809 PR?

@JamesNK
Copy link
Member

JamesNK commented Jul 16, 2024

Probably not. And there have been new APIs since then.

@mitchdenny
Copy link
Member

There are a few outstanding PRs with this but then I think we can probably call this issue substantially done (at least enough for now). Thanks to @Zombach, @bell-nat and @Alirexaa for their efforts here!

@DamianEdwards ?

@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
@JamesNK JamesNK reopened this Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication good first issue Good for newcomers help wanted Issue that is a good candidate for community contribution.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants