-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Use ArgumentException.ThrowIfNullOrEmpty #50666
Conversation
Thanks for your PR, @divyeshio. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
@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! |
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.. |
That's weird - that wildcard expression should be picking up our native projects. It's from Lines 117 to 121 in 36d30fd
|
@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 |
Building the whole repo with |
For following, should we change from Since we are checking for null here and ArgumentNullException make more sense for it. Secondly, there is no ThrowIfNull helper for ArgumentException. aspnetcore/src/Mvc/Mvc.ViewFeatures/src/Rendering/TagBuilder.cs Lines 45 to 48 in 6e49859
|
Seems reasonable, plus the current "or empty" message does not seem right. |
Using aspnetcore/src/Middleware/Rewrite/src/ApacheModRewrite/FlagParser.cs Lines 57 to 60 in ee6d623
After changes: ArgumentException.ThrowIfNullOrEmpty(flagString);
Failing test
Should I update the tests or is it big enough breaking change? |
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. |
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
Hi @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? |
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. |
Use
ArgumentException.ThrowIfNullOrEmpty
where applicableDescription
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