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.

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

Fix the TypeConverter when enabling UseSystemResourceKeys #68687

Merged
merged 3 commits into from
Apr 30, 2022
Merged

Fix the TypeConverter when enabling UseSystemResourceKeys #68687

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4dcb885
Fix TypeConverter when enabling UseSystemResourceKeys
tarekgh Apr 28, 2022
48e84a3
Address the feedback
tarekgh Apr 29, 2022
d977a03
Keep original behavior and enable running the added test with UseSyst…
tarekgh Apr 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ArrayConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class ArrayConverter : CollectionConverter
{
if (destinationType == typeof(string) && value is Array)
{
return SR.Format(SR.Array, value.GetType().Name);
return string.Format(SR.GetResourceString(nameof(SR.Array), "{0} Array"), value.GetType().Name);
}

return base.ConvertTo(context, culture, value, destinationType);
Expand Down
2 changes: 1 addition & 1 deletion ...ries/System.ComponentModel.TypeConverter/src/System/ComponentModel/CollectionConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public class CollectionConverter : TypeConverter
{
if (destinationType == typeof(string) && value is ICollection)
{
return SR.Collection;
return SR.GetResourceString(nameof(SR.Collection), "(Collection)");
}

return base.ConvertTo(context, culture, value, destinationType);
Expand Down
2 changes: 1 addition & 1 deletion ...ies/System.ComponentModel.TypeConverter/src/System/ComponentModel/CultureInfoConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public class CultureInfoConverter : TypeConverter
/// <summary>
/// Retrieves the "default" name for our culture.
/// </summary>
private static string DefaultCultureString => SR.CultureInfoConverterDefaultCultureString;
private static string DefaultCultureString => SR.GetResourceString(nameof(SR.CultureInfoConverterDefaultCultureString), "(Default)");

private const string DefaultInvariantCultureString = "(Default)";

Expand Down
2 changes: 1 addition & 1 deletion ...em.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerOptionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ public override PropertyDescriptorCollection GetProperties(ITypeDescriptorContex
{
if (destinationType == typeof(string))
{
return SR.CollectionConverterText;
return SR.GetResourceString(nameof(SR.CollectionConverterText), "(Collection)");
}
return base.ConvertTo(cxt, culture, value, destinationType);
}
Expand Down
2 changes: 1 addition & 1 deletion ...stem.ComponentModel.TypeConverter/src/System/ComponentModel/ExtendedPropertyDescriptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public override string DisplayName
string? providerName = site?.Name;
if (providerName != null && providerName.Length > 0)
{
name = SR.Format(SR.MetaExtenderName, name, providerName);
name = string.Format(SR.GetResourceString(nameof(SR.MetaExtenderName), "{0} on {1}"), name, providerName);
}
}
return name;
Expand Down
2 changes: 1 addition & 1 deletion ...s/System.ComponentModel.TypeConverter/src/System/ComponentModel/InstanceCreationEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace System.ComponentModel
/// </summary>
public abstract class InstanceCreationEditor
{
public virtual string Text => SR.InstanceCreationEditorDefaultText;
public virtual string Text => SR.GetResourceString(nameof(SR.InstanceCreationEditorDefaultText), "(New...)");

/// <summary>
/// This method is invoked when you user chooses the link displayed by the PropertyGrid for the InstanceCreationEditor.
Expand Down
2 changes: 1 addition & 1 deletion ...System.ComponentModel.TypeConverter/src/System/ComponentModel/MultilineStringConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public class MultilineStringConverter : TypeConverter

if (destinationType == typeof(string) && value is string)
{
return SR.Text;
return SR.GetResourceString(nameof(SR.Text), "(Text)");
}

return base.ConvertTo(context, culture, value, destinationType);
Expand Down
2 changes: 1 addition & 1 deletion ...aries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReferenceConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace System.ComponentModel
/// </summary>
public class ReferenceConverter : TypeConverter
{
private static readonly string s_none = SR.toStringNone;
private static readonly string s_none = SR.GetResourceString(nameof(SR.toStringNone), "(none)");
private readonly Type _type;

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ public virtual bool CanConvertTo(ITypeDescriptorContext? context, [NotNullWhen(t
/// </summary>
protected Exception GetConvertFromException(object? value)
{
string? valueTypeName = value == null ? SR.Null : value.GetType().FullName;
string? valueTypeName = value == null ? SR.GetResourceString(nameof(SR.Null), "(null)") : value.GetType().FullName;
throw new NotSupportedException(SR.Format(SR.ConvertFromException, GetType().Name, valueTypeName));
}

Expand All @@ -201,7 +201,7 @@ protected Exception GetConvertFromException(object? value)
/// </summary>
protected Exception GetConvertToException(object? value, Type destinationType)
{
string? valueTypeName = value == null ? SR.Null : value.GetType().FullName;
string? valueTypeName = value == null ? SR.GetResourceString(nameof(SR.Null), "(null)") : value.GetType().FullName;
throw new NotSupportedException(SR.Format(SR.ConvertToException, GetType().Name, valueTypeName, destinationType.FullName));
}

Expand Down
2 changes: 1 addition & 1 deletion ...raries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeListConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public override bool CanConvertTo(ITypeDescriptorContext? context, [NotNullWhen(
{
if (value == null)
{
return SR.none;
return SR.GetResourceString(nameof(SR.none), "(none)");
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion ...aries/System.ComponentModel.TypeConverter/src/System/Timers/TimersDescriptionAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public override string Description
if (!_replaced)
{
_replaced = true;
DescriptionValue = SR.Format(base.Description);
DescriptionValue = string.Format(base.Description);
tarekgh marked this conversation as resolved.
Show resolved Hide resolved
}
return base.Description;
}
Expand Down
12 changes: 6 additions & 6 deletions src/libraries/System.ComponentModel.TypeConverter/tests/ContainerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ public void Add2_Name_Duplicate()
Assert.Equal(typeof(ArgumentException), ex.GetType());
Assert.Null(ex.InnerException);
Assert.NotNull(ex.Message);
Assert.True(ex.Message.IndexOf("'dup'") != -1);
Assert.True(ex.Message.IndexOf("'dup'") != -1 || ex.Message.IndexOf(" dup") != -1);
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for omitting the apostrophes in the fallback string?

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, this is the exception message. Makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to me why this change is necessary. Can someone help me understand?

Copy link
Member

Choose a reason for hiding this comment

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

The format string from resources is: `Duplicate component name '{0}' ...``

The synthetized format string that you get with UseSystemResourceKeys is DuplicateComponentName {0}. Notice that the apostrophes are missing.

Copy link
Member

Choose a reason for hiding this comment

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

OK, that makes sense. But I guess I don't understand how this test is getting run with UseSystemResourceKeys=true.

Copy link
Member

Choose a reason for hiding this comment

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

I assume that @tarekgh has done a one-off manual run with UseSystemResourceKeys=true. I do not think we have it automated.

Copy link
Member Author

@tarekgh tarekgh Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That is right, I have run the test with and without enabling UseSystemResourceKeys manually and ensured it is passing in both ways. Enabling UseSystemResourceKeys in general should be done separately from this PR. I'll open issue to track that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have logged the issue #68707 to track enabling UseSystemResourceKeys on libraries tests.

Assert.Null(ex.ParamName);
Assert.Equal(1, container.Components.Count);

Expand All @@ -334,7 +334,7 @@ public void Add2_Name_Duplicate()
Assert.Equal(typeof(ArgumentException), ex.GetType());
Assert.Null(ex.InnerException);
Assert.NotNull(ex.Message);
Assert.True(ex.Message.IndexOf("'duP'") != -1);
Assert.True(ex.Message.IndexOf("'duP'") != -1 || ex.Message.IndexOf(" duP") != -1);
Assert.Null(ex.ParamName);
Assert.Equal(1, container.Components.Count);

Expand All @@ -356,7 +356,7 @@ public void Add2_Name_Duplicate()
Assert.Equal(typeof(ArgumentException), ex.GetType());
Assert.Null(ex.InnerException);
Assert.NotNull(ex.Message);
Assert.True(ex.Message.IndexOf("'dup'") != -1);
Assert.True(ex.Message.IndexOf("'dup'") != -1 || ex.Message.IndexOf(" dup") != -1);
Assert.Null(ex.ParamName);
Assert.Equal(2, container.Components.Count);
Assert.Equal(1, container2.Components.Count);
Expand Down Expand Up @@ -729,7 +729,7 @@ public void ValidateName_Name_Duplicate()
Assert.Equal(typeof(ArgumentException), ex.GetType());
Assert.Null(ex.InnerException);
Assert.NotNull(ex.Message);
Assert.True(ex.Message.IndexOf("'dup'") != -1);
Assert.True(ex.Message.IndexOf("'dup'") != -1 || ex.Message.IndexOf(" dup") != -1);
Assert.Null(ex.ParamName);
Assert.Equal(2, _container.Components.Count);
_container.InvokeValidateName(compB, "whatever");
Expand All @@ -742,7 +742,7 @@ public void ValidateName_Name_Duplicate()
Assert.Equal(typeof(ArgumentException), ex.GetType());
Assert.Null(ex.InnerException);
Assert.NotNull(ex.Message);
Assert.True(ex.Message.IndexOf("'dup'") != -1);
Assert.True(ex.Message.IndexOf("'dup'") != -1 || ex.Message.IndexOf(" dup") != -1);
Assert.Null(ex.ParamName);
Assert.Equal(2, _container.Components.Count);
_container.InvokeValidateName(compC, "whatever");
Expand All @@ -757,7 +757,7 @@ public void ValidateName_Name_Duplicate()
Assert.Equal(typeof(ArgumentException), ex.GetType());
Assert.Null(ex.InnerException);
Assert.NotNull(ex.Message);
Assert.True(ex.Message.IndexOf("'dup'") != -1);
Assert.True(ex.Message.IndexOf("'dup'") != -1 || ex.Message.IndexOf(" dup") != -1);
Assert.Null(ex.ParamName);
Assert.Equal(2, _container.Components.Count);
_container.InvokeValidateName(compD, "whatever");
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.ComponentModel.TypeConverter/tests/CultureInfoConverterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Tests;
using Xunit;

namespace System.ComponentModel.Tests
Expand Down Expand Up @@ -152,6 +153,15 @@ public void GetCultureName_Overriden_ConversionsReturnsExpected()
Assert.Equal("Fixed", converter.ConvertTo(new CultureInfo("en-US"), typeof(string)));
}

[Fact]
public void CultureInfoConverterForDefaultValue()
{
using (new ThreadCultureChange(null, CultureInfo.InvariantCulture))
{
Assert.Equal("", ((CultureInfo)TypeDescriptor.GetConverter(typeof(System.Globalization.CultureInfo)).ConvertFrom(null, null, "(Default)")).Name);
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to run this test with UseSystemResourceKeys=true in order to test the bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have manually run the test with and without UseSystemResourceKeys. As @jkotas indicated, such mode should be tested for all libraries and not just this one only. I'll open infrastructure issue to do that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure when we would enable a full test run with this setting. It doesn't seem like something that will happen anytime soon.

For now, we can run this one test with both UseSystemResourceKeys=true or false by doing something like the following code, only with the UseSystemResourceKeys switch:

runtime/src/libraries/System.Diagnostics.Tracing/tests/BasicEventSourceTest/TestNotSupported.cs

Lines 18 to 30 in 66bd8b1

/// <summary>
/// Tests using EventSource when the System.Diagnostics.Tracing.EventSource.IsSupported
/// feature switch is set to disable all EventSource operations.
/// </summary>
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void TestBasicOperations_IsSupported_False()
{
RemoteInvokeOptions options = new RemoteInvokeOptions();
options.RuntimeConfigurationOptions.Add("System.Diagnostics.Tracing.EventSource.IsSupported", false);
RemoteExecutor.Invoke(() =>
{
using (var log = new EventSourceTest())

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, will do it.

}
}

private class SubCultureInfoConverter : CultureInfoConverter
{
public new string GetCultureName(CultureInfo culture)
Expand Down