Cleanup TypeConverter replacements #203

hughbe · 2018-12-22T23:22:01Z

Delete DateTimeValueSerializerContext as it is a) unused in the DateTimeValueSerializer base class and it has b) absolutely no functionality - passing null will suffice!
Put DateTimeOffsetConverter2 in the right namespace
Light code modernization and cleanup

xref with #200

hughbe · 2018-12-22T23:27:32Z

src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Replacements/DateTimeOffsetConverter2.cs

@@ -64,11 +60,13 @@ public override object ConvertTo(ITypeDescriptorContext context, CultureInfo cul
                        intType,
                        intType,
                        typeof(TimeSpan)
+                    });
+
+                    if (constructor == null)


I think this is dead code. This API has existed basically forever and we'll always be able to get it right? https://apisof.net/catalog/System.DateTimeOffset..ctor(Int32,Int32,Int32,Int32,Int32,Int32,TimeSpan)

I say remove it but please confirm :)

@dipeshmsft what do you reckon?

I will take a look at this PR as well. I will ping you back once reviewed.

hughbe · 2023-05-03T09:16:14Z

@dipeshmsft I will try to break this up to some more commits to provide more context :)

dipeshmsft · 2023-06-07T05:36:52Z

/azp run

azure-pipelines · 2023-06-07T05:37:02Z

Azure Pipelines successfully started running 1 pipeline(s).

dipeshmsft · 2023-06-07T09:40:47Z

@hughbe can you take a look at the build failure for this PR ?

lindexi · 2023-06-08T08:53:38Z

src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Replacements/TypeListConverter.cs

-                    types[i] = (Type)typeTypeConverter.ConvertFrom(context, TypeConverterHelper.InvariantEnglishUS, tl[i]);
-                }
-                return types;
+                throw new NullReferenceException();


I think this code may be controversial, why throw a null exception when the variable is not null?

Yeah - basically this always used to throw NullReferenceException, because StringHelpers.SplitTypeList always returns null and then tl.Length would throw NullReferenceException.

Welcome comments here.

Hey @hughbe ,
We are considering merging this PR. I'd like to offer a suggestion: rather than removing the code and returning null , we could set it up like this:
public static string[] SplitTypeList(string typeList) => Array.Empty<string>();

With this approach, StringHelpers.SplitTypeList will return an empty string, allowing the code to proceed. This setup also leaves the door open for us to potentially utilize this feature in the future.

Thank you for your consideration.

OK - have changed this to return Array.Empty()

hughbe · 2023-06-09T11:02:29Z

src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/Markup/DateTimeConverter2.cs


        public override object ConvertTo(ITypeDescriptorContext context, CultureInfo culture, object value, Type destinationType)
        {
            if (destinationType != null && value is DateTime)
            {
-                _dateTimeValueSerializer.ConvertToString( value as string, _valueSerializerContext );
+                _dateTimeValueSerializer.ConvertToString(value as string, null);


🚩 this doesn't return the actual value?!

@rchauhan18

@hughbe My apologies for missing this while reviewing. It appears that this should return:
return _dateTimeValueSerializer.ConvertToString(value as string, null);

Regarding the build failure, we cannot define StringHelpers as private.

If you would like, I can add a commit with the above changes, as they are very small.

hughbe · 2023-06-09T11:03:16Z

src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Replacements/DateTimeConverter2.cs

-        {
-            return _dateTimeValueSerializer.ConvertFromString( value as string, _valueSerializerContext );
-        }
+            => _dateTimeValueSerializer.ConvertFromString(value as string, null);


Since DateTimeValueSerializerContext doesn't have any functionality, we can just pass null and remove the class here and in the other DateTimeConverter2

hughbe · 2023-06-09T11:03:34Z

src/Microsoft.DotNet.Wpf/src/System.Xaml/System/Xaml/Replacements/DateTimeOffsetConverter2.cs

                {
+                    // Use the year, month, day, hour, minute, second, millisecond, offset constructor
+                    ConstructorInfo constructor = typeof(DateTimeOffset).GetConstructor(new Type[]


constructor will always exist, remove null check

Update DateTimeConverter2.cs

- Remove dead `if (constructor != null)` check - this ctor will always be there. - Cleanup code

- Code cleanup

rchauhan18 · 2023-10-05T06:15:44Z

Thanks @hughbe for your contribution.

hughbe commented Dec 22, 2018

View reviewed changes

hughbe force-pushed the typeconverter-rep branch from dfa9b3c to 1acb2db Compare December 23, 2018 00:06

grubioe added the PR metadata: Label to tag PRs, to facilitate with triage label May 28, 2019

hughbe force-pushed the typeconverter-rep branch from 1acb2db to d7386ad Compare June 26, 2019 11:31

Base automatically changed from master to main March 17, 2021 17:38

ryalanms requested a review from a team as a code owner March 17, 2021 17:38

pchaurasia14 added the Community Contribution A label for all community Contributions label Jul 20, 2022

ghost assigned hughbe Jul 20, 2022

hughbe force-pushed the typeconverter-rep branch from d7386ad to ecce08d Compare May 2, 2023 14:35

dipeshmsft self-assigned this May 3, 2023

hughbe force-pushed the typeconverter-rep branch from ecce08d to 1b5451c Compare May 3, 2023 09:47

lindexi reviewed Jun 8, 2023

View reviewed changes

hughbe force-pushed the typeconverter-rep branch from 5c9fd9c to 5e3f333 Compare June 9, 2023 11:01

hughbe commented Jun 9, 2023

View reviewed changes

hughbe force-pushed the typeconverter-rep branch from 5e3f333 to c11682f Compare September 6, 2023 07:27

hughbe force-pushed the typeconverter-rep branch 2 times, most recently from 2064e14 to 8b55ab8 Compare September 20, 2023 08:59

hughbe added 5 commits September 20, 2023 11:46

Remove dead code from DateTimeConverter2

ab0b3b2

Update DateTimeConverter2.cs

Cleanup DateTimeOffsetConverter2

a3c0fba

- Remove dead `if (constructor != null)` check - this ctor will always be there. - Cleanup code

Cleanup TypeUriConverter

f5ed558

- Code cleanup

Cleanup TypeListConverter

6902413

Cleanup other Xaml Replacement converters

ced5cdd

hughbe force-pushed the typeconverter-rep branch from 8b55ab8 to ced5cdd Compare September 20, 2023 10:46

rchauhan18 added the Queue for test pass label Sep 20, 2023

rchauhan18 added Included in test pass and removed Queue for test pass labels Oct 5, 2023

rchauhan18 merged commit d05df2b into dotnet:main Oct 5, 2023
9 checks passed

hughbe deleted the typeconverter-rep branch October 5, 2023 06:56

ghost locked as resolved and limited conversation to collaborators Nov 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup TypeConverter replacements #203

Cleanup TypeConverter replacements #203

hughbe commented Dec 22, 2018 •

edited

hughbe Dec 22, 2018

hughbe May 2, 2023

dipeshmsft May 3, 2023

hughbe commented May 3, 2023

dipeshmsft commented Jun 7, 2023

azure-pipelines bot commented Jun 7, 2023

dipeshmsft commented Jun 7, 2023

lindexi Jun 8, 2023

hughbe Jun 9, 2023

rchauhan18 Sep 15, 2023

hughbe Sep 20, 2023

hughbe Jun 9, 2023

hughbe Sep 20, 2023

rchauhan18 Sep 20, 2023

hughbe Jun 9, 2023

hughbe Jun 9, 2023

rchauhan18 commented Oct 5, 2023

Cleanup TypeConverter replacements #203

Cleanup TypeConverter replacements #203

Conversation

hughbe commented Dec 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented May 3, 2023

dipeshmsft commented Jun 7, 2023

azure-pipelines bot commented Jun 7, 2023

dipeshmsft commented Jun 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchauhan18 commented Oct 5, 2023

hughbe commented Dec 22, 2018 •

edited