fixed resolving json converters (included issue #75)

[Erifirin]

added explicit ordering of assemblies

[Erifirin]

tests failed due to "Setup Unity license" issues

[applejag]

added explicit ordering of assemblies

Thanks for creating this PR! But what is the issue you're trying to solve here?

The code already does a .Reverse() to set some sort of ordering. So this new .Order(Order) seems a bit redundant.

tests failed due to "Setup Unity license" issues

Yes sorry. The unit tests require a sensitive value, which is not available when the PR is created by an outside contributor. When your PR is done, then I'll have to recreate it just so the tests can run

[Erifirin]

The order of assemblies in array is non-deterministic. In my case this leads to conflicts between convetrers from this repo and the same convetrers embedded in RiderFlow assemblies (as internal classes). Calling Reverse() doesn't solve the problem because it just reverses the array. In result, RiderFlow converters appear at top of the array and break everything.

My fix ensures that Newtonsoft.Json assemblies will be placed at the beginning of the array and mscorlib is at the end. I also left Reverse() so as not to break anything. But it seems useless to me.

[Erifirin]

Ok, let's look what TypeCache._assemblies contains right now in my project (without fix - reversed array): reverse.txt JetBrains.RiderFlow.Repacked assembly (with internal converters) is closer to beginning of the array than any Newtonsoft.Json. And this is a problem.

At the same time, this is what's written in the comments:

            // Reversing so we get last imported assembly first.
            // When searching for types we want to look in mscorlib last
            // and Newtonsoft.Json up as the first ones

To be honest, I don’t understand how a simple Reverse() can guarantee such sorting. Obviously, it can't. That's why i sort the array explicitly (reversed array + order fix): reverse_and_order.txt.

But I agree that two sorts seems a bit redundant. I would remove Reverse() if its only purpose is to move Newtonsoft.Json and mscorlib. Here another file with TypeCache._assemblies content (wtih order, without reverse):
order.txt

[applejag]

Ok, let's look what TypeCache._assemblies contains right now in my project (without fix - reversed array): reverse.txt JetBrains.RiderFlow.Repacked assembly (with internal converters) is closer to beginning of the array than any Newtonsoft.Json. And this is a problem.

At the same time, this is what's written in the comments:

            // Reversing so we get last imported assembly first.
            // When searching for types we want to look in mscorlib last
            // and Newtonsoft.Json up as the first ones

To be honest, I don’t understand how a simple Reverse() can guarantee such sorting. Obviously, it can't. That's why i sort the array explicitly (reversed array + order fix): reverse_and_order.txt.

But I agree that two sorts seems a bit redundant. I would remove Reverse() if its only purpose is to move Newtonsoft.Json and mscorlib. Here another file with TypeCache._assemblies content (wtih order, without reverse): order.txt

You're completely right, it's a bit redundant. And yes, that comment makes no sense.

But just looking at the issue here, it seems that maybe better type resolution would improve this too. We're only looking up the type by type name right now, but what if we also looked it up on assembly name?

When I wrote the serializableobject and editor thing, I made it only store the type name, as I noticed it would otherwise frequently forget my converter settings whenever the assembly changed (such as when just the version changed). One solution would be to store the assembly name separately, and then first try to look up on a specific assembly, but after that try look up on only the type name in any assembly.

I've pushed some changes that should make type lookups much more stable.

[applejag]

Wdyt? @Erifirin

[Erifirin]

Yes! Now it works as expected.
Your solution definitely looks better. Also if you don't mind, I would get rid of multiple calls assembly.GetName() to reduce allocations.

[applejag]

Yes! Now it works as expected. Your solution definitely looks better. Also if you don't mind, I would get rid of multiple calls assembly.GetName() to reduce allocations.

Sure! I did some further refactorings. Interesting asynchronous pair-programming we had there.

Do you have any other changes in mind? Otherwise we can go ahead and merge this.

[Erifirin]

Do you have any other changes in mind?

No, I think it's ready to merge :).
Thank you for your assistance.