Use ArgumentException.ThrowIfNullOrEmpty

[divyeshio]

Use ArgumentException.ThrowIfNullOrEmpty where applicable

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

As suggested here #48414 (comment) , I have splitted up the changes specific to ArgumentException.ThrowIfNullOrEmpty and have already addressed the feedback in this PR.

Also, I have made changes to Resources.ArgumentCannotBeNullOrEmpty in the separate commit (just easier to filter on commit and review)

Fixes #50665

[ghost]

Thanks for your PR, @divyeshio. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[captainsafia]

@divyeshio Thanks! It looks like you might've inadvertently re-introduced changes to the spa-templates submodule that are causing the build break. Removing these changes from the commit should do the trick. Let me know if you need help with this!

[danmoseley]

thanks for the PR @divyeshio . Looks like there's a build error -- typo. Could you please build locally before pushing changes? That will save unnecessary runs of our (considerable) validation matrix..

[divyeshio]

While restoring, I'm getting following error. I have tried deleting artifacts folder, cleared everything in nuget cache but still failing, all while being on main branch. Can you help me with this?

[wtgodbe]

That's weird - that wildcard expression should be picking up our native projects. It's from

aspnetcore/eng/Build.props

Lines 117 to 121 in 36d30fd

	<ItemGroup Condition=" '$(TargetOsName)' == 'win' AND ('$(TargetArchitecture)' == 'x86' OR '$(TargetArchitecture)' == 'x64') ">
	<NativeProjects Include="$(RepoRoot)src\**\*.vcxproj" Exclude="@(ProjectToExclude)" AdditionalProperties="Platform=x64" />
	<NativeProjects Include="$(RepoRoot)src\**\*.vcxproj" Exclude="@(ProjectToExclude)" AdditionalProperties="Platform=Win32" />
	<NativeProjects Include="$(RepoRoot)src\**\*.vcxproj" Exclude="@(ProjectToExclude)" AdditionalProperties="Platform=arm64" />
	</ItemGroup>

. It must be something in your local setup - have you deleted any files? What command-line shell are you using? Worst case scenario, you could work around it by deleting the block I linked (but don't include that change in your PR)

[divyeshio]

@wtgodbe It's resolved now, I had to update vs workload, delete the repo, start over again and then it worked. Btw, I'm using powershell.

A question, as these changes are spread across several projects, how do I verify my changes? Do I need to build and test whole repo? I tried -projects parameter in eng/build script, build fails when I use that vs when I build the individual project by navigating to the folder, it builds fine.

[wtgodbe]

Building the whole repo with eng/build.cmd -test will verify that everything is working. You can also run tests for the changed projects with dotnet test in the project directory

[divyeshio]

For following, should we change from ArgumentException to ArgumentNullException ?

Since we are checking for null here and ArgumentNullException make more sense for it. Secondly, there is no ThrowIfNull helper for ArgumentException.
And change it to ArgumentNullException.ThrowIfNull(tagBuilder)

aspnetcore/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs

Lines 45 to 48 in 6e49859

	if (tagBuilder == null)
	{
	throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(tagBuilder));
	}

[danmoseley]

For following, should we change from ArgumentException to ArgumentNullException ?

Seems reasonable, plus the current "or empty" message does not seem right.

[divyeshio]

Using ArgumentException.ThrowIfNullOrEmpty throws ArgumentException and ArgumentNullException. Methods that were simply throwing ArgumentException have their Tests failing after changes, as tests were expecting ArgumentException but got ArgumentNullException instead. One such example is following:

aspnetcore/src/Middleware/Rewrite/src/ApacheModRewrite/FlagParser.cs

Lines 57 to 60 in ee6d623

	if (string.IsNullOrEmpty(flagString))
	{
	throw new ArgumentException("Argument cannot be null or empty string.", nameof(flagString));
	}

After changes: ArgumentException.ThrowIfNullOrEmpty(flagString);

Failing test

aspnetcore/src/Middleware/Rewrite/test/ApacheModRewrite/FlagParserTest.cs

Line 61 in ee6d623

Assert.Throws<ArgumentException>(() => FlagParser.Parse(null));

Should I update the tests or is it big enough breaking change?

[danmoseley]

I think it's fine to update tests that were expecting AE for a null parameter to now expect ANE. We've done that before.

[ghost]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[ghost]

Hi @divyeshio.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

[divyeshio]

Hi @danmoseley, I am still willing to make this contribution happen. I have few commits unpushed and last time I remember there were still few test cases failing and required changes. I was almost 80-90% there with all the changes. Now, starting again, Do I need to do something rebase or anything?

[ghost]

Hi @divyeshio. 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.