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

Fix Issue #41822 Add IsNullOrEmpty or IsNullOrWhiteSpace check for method parameters throws an empty "message" #42101

Merged
14 commits merged into from
Apr 21, 2020

Conversation

Sliptory
Copy link
Contributor

@Sliptory Sliptory commented Mar 2, 2020

@Sliptory Sliptory requested a review from a team as a code owner March 2, 2020 20:41
@jinujoseph jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 4, 2020
@Sliptory Sliptory requested a review from CyrusNajmabadi March 26, 2020 23:23
Copy link
Contributor Author

@Sliptory Sliptory left a comment

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

@Sliptory Sliptory left a comment

Choose a reason for hiding this comment

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

need review

@@ -1460,6 +1460,16 @@ This version used in: {2}</value>
<data name="Implement_explicitly" xml:space="preserve">
<value>Implement explicitly</value>
</data>
<data name="cannot_be_null_or_empty" xml:space="preserve">
<value>cannot be null or empty</value>
Copy link
Member

Choose a reason for hiding this comment

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

Please add a {0} place holder for replacement with the parameter name. Constructing this string with concatenation may not make sense for all languages.

@@ -330,17 +330,17 @@ public C(string a, string b, string c)
{
if (string.IsNullOrEmpty(a))
{
throw new ArgumentException(""message"", nameof(a));
throw new ArgumentException($""'{nameof(a)}' cannot be null or empty"", nameof(a));
Copy link
Member

Choose a reason for hiding this comment

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

tests will fail on spanish leg if they reference english strings.

<data name="Struct_contains_assignment_to_this_outside_of_constructor_Make_readonly_fields_writable" xml:space="preserve">
<value>Struct contains assignment to 'this' outside of constructor. Make readonly fields writable</value>
<comment>{Locked="Struct"}{Locked="this"} these are C#/VB keywords and should not be localized.</comment>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

why was this added?

return generator.ObjectCreationExpression(
GetTypeNode(compilation, generator, typeof(ArgumentException)),
generator.LiteralExpression("message"),
generator.InterpolatedStringExpression(
generator.CreateInterpolatedStringStartToken(false).WithLeadingTrivia(content.First().GetLeadingTrivia()),
Copy link
Member

Choose a reason for hiding this comment

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

named arg for false

Copy link
Member

Choose a reason for hiding this comment

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

the first/last content nodes won't have any trivia. so i don't know why w're taking trivia from them and adding to the interpolated string.

<source>{0} can be simplified</source>
<target state="translated">{0} se dá zjednodušit.</target>
<note />
</trans-unit>
Copy link
Member

Choose a reason for hiding this comment

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

why was this aded?

@CyrusNajmabadi
Copy link
Member

@Sliptory If you'd like, we can pick up this PR from you and finish it. Thanks!

@Sliptory
Copy link
Contributor Author

@Sliptory If you'd like, we can pick up this PR from you and finish it. Thanks!

Ok, Thanks!)

@CyrusNajmabadi
Copy link
Member

@JoeRobich i can take this unless you wanted to :)

@JoeRobich
Copy link
Member

@JoeRobich i can take this unless you wanted to :)

@CyrusNajmabadi Please, be my guest =)

@CyrusNajmabadi
Copy link
Member

@JoeRobich this is ready for review. Thanks!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 36bb0e5 into dotnet:master Apr 21, 2020
@CyrusNajmabadi
Copy link
Member

Thanks @Sliptory . Sorry that took so long :)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE auto-merge Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants