Skip to content

Refactor "Routing/Builder" methods pre conditions #41783

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 25, 2022
Merged

Refactor "Routing/Builder" methods pre conditions #41783

merged 6 commits into from
May 25, 2022

Conversation

mottaghipour
Copy link
Contributor

Refactor "Routing/Builder" methods pre conditions

Replace block conditions with "ArgumentNullException.ThrowIfNull(...);"

@mottaghipour mottaghipour requested a review from javiercn as a code owner May 21, 2022 03:53
@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels May 21, 2022
@dnfadmin
Copy link

dnfadmin commented May 21, 2022 •
edited
Loading

CLA assistant check
All CLA requirements met.

davidfowl
davidfowl reviewed May 21, 2022
View reviewed changes
src/Http/Routing/src/Builder/RoutingEndpointConventionBuilderExtensions.cs Outdated
{
throw new ArgumentNullException(nameof(items));
}
ArgumentNullException.ThrowIfNull(builder, nameof(builder));
Copy link
Member

Choose a reason for hiding this comment

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

Why is nameof needed? Isn't it inferred by default from the expression passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. You are right and I was wrong.
@halter73 Had added MapGroup(#41265) pre conditions with param names and I continued the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just got fixed

@mottaghipour mottaghipour reopened this May 21, 2022
@ghost
Copy link

ghost commented May 21, 2022

Hi @mottaghipour. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@halter73
Copy link
Member

halter73 commented May 24, 2022 •
edited
Loading

@mottaghipour Why did you close this PR? s there some reason you do not want us to merge this change?

@halter73 halter73 reopened this May 24, 2022
@mottaghipour
Copy link
Contributor Author

@halter73 After pulling, I read the contributing guidelines and in this part, I realized that my work was wrong, refactoring here is bad for merge!
But if you do not have a problem, merge this change. I am also happy for my contribution :)

@halter73
Copy link
Member

Generally, we like people to file an issue first. Mostly it's so people don't waste time on PRs that we won't merge. In this particular case it's fine though. Thanks for your contribution!

@halter73 halter73 merged commit 19d21ad into dotnet:main May 25, 2022
@ghost ghost added this to the 7.0-preview6 milestone May 25, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@javiercn javiercn javiercn approved these changes

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0-preview6
Development

Successfully merging this pull request may close these issues.
6 participants
@mottaghipour @dnfadmin @halter73 @davidfowl @javiercn @amcasey