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 cross framework incompatibility for System.Drawing.Color #208

Merged

Conversation

Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented Mar 25, 2021

Closes #117
Closes #206

@@ -123,6 +123,11 @@ private static Type GetTypeFromManifestName(Stream stream, DeserializerSession s
{
shortName = shortName.Replace("System.Private.CoreLib,%core%", "mscorlib,%core%");
}

if (shortName.Contains("System.Drawing.Primitives"))
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is another case of where a single namespace was changed between .NET framework and .NET core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They decided that structs are primitives and moved all structs inside System.Drawing into System.Drawing.Primitives

_serializer = new Serializer(new SerializerOptions(
packageNameOverrides: new List<CrossPlatformPackageNameOverride>
{
new CrossPlatformPackageNameOverride("System.Drawing.Primitives", ".Primitives", "")
Copy link
Member

Choose a reason for hiding this comment

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

So how does this work exactly @Arkatufus ? What's the formula for substituting names in one package for another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first string is the "fingerprint", the unique string that indicates the wrong package name. It is matched using string.Contain.
The second string is the string pattern that needs to be replaced.
The third is the string to replace it with.

In this example, "if the package name contains the string System.Drawing.Primitives, replace .Primitives with an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

How could we make that more intuitive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Think passing in a Func<string,string> predicate would make more sense? Leave it up to the user how to transform the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll make the changes

Copy link
Contributor Author

@Arkatufus Arkatufus Apr 12, 2021

Choose a reason for hiding this comment

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

oh... I just remembered... we can't do that, it has to be a set of strings, because this will have to be configurable from hocon

Copy link
Member

Choose a reason for hiding this comment

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

We can always pass in a Setup for this for Hyperion just like we did for Newtonsoft.Json .

Copy link
Member

Choose a reason for hiding this comment

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

And you can add a layer that translates HOCON into these delegates too but for this level of customization passing in an array of typed functions is probably better.

{
shortName = shortName.Replace("mscorlib,%core%", "System.Private.CoreLib,%core%");
shortName = adapter(shortName);
if (!ReferenceEquals(oldName, shortName))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optimization, bail out as soon as there is a match, we will only allow one transformation.

Copy link
Member

Choose a reason for hiding this comment

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

I like it

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Looks great - this should help quite a bit with handling some of these .NET Framework vs. NET Core conversions for users who are migrating.

_serializer = new Serializer(new SerializerOptions(
packageNameOverrides: new List<Func<string, string>>
{
str => str.Contains("System.Drawing.Primitives") ? str.Replace(".Primitives", "") : str
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

return new List<Func<string, string>>
{
#if NET45
str => str.Contains("System.Private.CoreLib,%core%")
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot deserialize types when simplified qualified name is not resolvable by .NET framework
2 participants