YamlDotNet icon indicating copy to clipboard operation
YamlDotNet copied to clipboard
verified publisher icon aaubry

Asset store update

Open steinbitglis opened this issue 4 years ago • 6 comments

Hey Antoine! This is not a pull request that I actually want you to merge anywhere, but I'm in need of a little bit of code review. I've held back on updating the Unity Asset Store version of YamlDotNet, because C# 8 features were being used after version 6.1.2. But now that Unity has a much upgraded compiler that supports C# 8, I was making another attempt, just to stay on par with your progress. Unity now supports .net standard 2.0. There were a few things in YamlDotNet 11.2.1 that required .net standard 2.1, but I think it's pretty close.

So what this is, is just me asking if you could review my branch unity_compatible, and see if you think I broke anything. I don't have a proper IDE set up for the project, and I'm not able to run the test suite. It would be a really big job for me just to run the tests, but if it's easier for you, could you do a quick review and maybe a sanity check (if I've misunderstood the .net standard 2.1 features or something.). The sample code seems to run alright in Unity, but I haven't checked the output properly.

If you think it looks ok, I'll go ahead and package it up for the asset store, and probably also figure out which exact Unity version is recent enough for this updated code.

steinbitglis avatar Sep 14 '21 20:09 steinbitglis

Hi!

Nice work! I'll try to build and review all this tomorrow, then I'll let you know if I found any problem.

Sep 14, 2021 21:38:36 Fredrik Ludvigsen @.***>:

Hey Antoine! This is not a pull request that I actually want you to merge anywhere, but I'm in need of a little bit of code review. I've held back on updating the Unity Asset Store version of YamlDotNet, because C# 8 features were being used after version 6.1.2. But now that Unity has a much upgraded compiler that supports C# 8, I was making another attempt, just to stay on par with your progress. Unity now supports .net standard 2.0. There were a few things in YamlDotNet 11.2.1 that required .net standard 2.1, but I think it's pretty close.

So what this is, is just me asking if you could review my branch unity_compatible, and see if you think I broke anything. I don't have a proper IDE set up for the project, and I'm not able to run the test suite. It would be a really big job for me just to run the tests, but if it's easier for you, could you do a quick review and maybe a sanity check (if I've misunderstood the .net standard 2.1 features or something.). The sample code seems to run alright in Unity, but I haven't checked the output properly.

If you think it looks ok, I'll go ahead and package it up for the asset store, and probably also figure out which exact Unity version is recent enough for this updated code.

You can view, comment on, or merge this pull request online at:

  https://github.com/aaubry/YamlDotNet/pull/634

Commit Summary

  • Getting rid of a dependency that is not needed for running sample code in Unity.

  • Getting rid of .net standard 2.1 specific features, so compilation in Unity will work.

File Changes

  • M YamlDotNet.Samples/ConvertYamlToJson.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-3b06ea00b531514b9eac91ee91a6be166075cc78a9014acfdd4f2efca84ce374] (1)

  • M YamlDotNet.Samples/DeserializeObjectGraph.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-10c069c563c0cd34bb7521ac078f24d0f8de647569a5a9f983f5378a1e39c707] (2)

  • M YamlDotNet.Samples/DeserializingMultipleDocuments.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-4ce9139af682fcfbb5cb185140180bf6e9fc25c4612d5e2384e4e361a1c65b4b] (1)

  • M YamlDotNet.Samples/LoadingAYamlStream.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-0c7f0ce46592574c44919b8f018178d8bda03a41b19c13aaaf71852339d7e741] (1)

  • M YamlDotNet.Samples/SerializeObjectGraph.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-c73c6ee3b97c20d8c1c7f66f8635fcfa096e06612e09464ba9bbfbb6d511a1f4] (29)

  • M YamlDotNet/Core/ParserExtensions.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-411e4df01adf4d06fe9e38f19cba345aa43cb5339f62669a3efa790aa87d8cc3] (8)

  • M YamlDotNet/Helpers/ExpressionExtensions.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-fa765ab095deedab31d2f7562bbaf15e21687517f89683d0e636a77664f5ffea] (2)

  • M YamlDotNet/Helpers/OrderedDictionary.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-024174774d0d9f6006237416ccdb52dc6aa387d3cb54025e5387672305ad2bad] (2)

  • M YamlDotNet/RepresentationModel/DocumentLoadingState.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-c0fcac41eb441147a9f17c4160fe8076f5df7077b22eb82e4cdb1444e3a43582] (2)

  • M YamlDotNet/RepresentationModel/YamlNodeIdentityEqualityComparer.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-e89b3e6e873c5449bbdcd50daeedc494dc97a0e2e38e92f8b2dcf06e4401d4af] (2)

  • M YamlDotNet/Serialization/ITypeInspector.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-dfa570d4bfe06be9f159f477ad1a2fecce39dc7a55fa0ae9b359c5a3855b2dac] (2)

  • M YamlDotNet/Serialization/TypeInspectors/TypeInspectorSkeleton.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-2c0d6eb9c9637885cce30a8ed4c103a3e056042076fe596c94a44286b00600a8] (2)

  • M YamlDotNet/Serialization/Utilities/ObjectAnchorCollection.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-a4fd00a9f39300d77333e4891f4f16bf1aa398dddcb3c1e56d550be0d01c8969] (2)

  • M YamlDotNet/Serialization/YamlAttributeOverrides.cs[https://github.com/aaubry/YamlDotNet/pull/634/files#diff-911ec3d5d9c95d1ac500702224f4d42a42f7049881d57f399a50fa217abc04b1] (2)

Patch Links:

  • https://github.com/aaubry/YamlDotNet/pull/634.patch

  • https://github.com/aaubry/YamlDotNet/pull/634.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub[https://github.com/aaubry/YamlDotNet/pull/634], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAEZ5XYQGGR7KFROVTXQMA3UB6XEVANCNFSM5EA6UPFA]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEQAAABECAYAAAA4E5OyAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAoSURBVHic7cEBAQAAAIIg/69uSEABAAAAAAAAAAAAAAAAAAAAAACPBkiEAAHzeIrHAAAAAElFTkSuQmCC###24x24:true###][Tracking image][https://github.com/notifications/beacon/AAEZ5X2LKGHQH53R7MC32TTUB6XEVA5CNFSM5EA6UPFKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4O3EBJLQ.gif]

aaubry avatar Sep 16 '21 21:09 aaubry

I see that you have removed a bunch of nullable annotations, such as [MaybeNullWhen(true)]. Those are needed for other platforms, so I would like to avoid removing them. Can you explain how you are compiling this with Unity so that I can try a fix that works for all cases?

aaubry avatar Sep 17 '21 21:09 aaubry

Yes, I've exported a unitypackage that you can look at. YamlDotNet_as_unitypackage.zip

Unzip, then import the package from "Assets"->"Import Package"->"Custom Package". There is a file called YamlDotNet.asmdef, this makes unity compile the folder and everything beneath it into one assembly.

Create a new game object in a scene, and put an ExampleRunner on it. Press play.

The newer versions of Unity will be compiling to .net standard 2.0

steinbitglis avatar Sep 18 '21 21:09 steinbitglis

Did you manage to run the examples in Unity? Let me know if I can help further.

steinbitglis avatar Sep 27 '21 16:09 steinbitglis

Did you manage to run the examples in Unity? Let me know if I can help further.

I'm trying right now. I'll let you know how it goes.

aaubry avatar Sep 28 '21 20:09 aaubry

So, I followed your instructions and was able to build a project with your package. Then, I replaced the code from your package and could see the compilation problems, which I was able to fix. I have already pushed this commit that contains the changes needed to compile the code.

Then, I created a new, empty Unity project in the YamlDotNet directory and added the YamlDotNet code by creating symlinks to include the necessary files and directories. This way, I was able to have a working Unity project that uses the code directly from the main repository. The advantage of this is that anyone can check for Unity copatibility issues while working on the project.

This also seems useful as a step to produce the unity package, although I don't know how that step is performed. I have opened a pull request so that you can check this and maybe make any changes needed to create the unity package from there.

Please have a look and let me know if this is useful. Keep in mind that, if you're on Windows, you will need to enable symlinks on Git, by setting core.symlinks to true, as explained here.

aaubry avatar Sep 28 '21 23:09 aaubry

This PR appears to be abandoned, it's over a year old. I'm going to close it. There is a couple feature requests for Unity related packages/processes which I'm planning on looking in to in the near future.

EdwardCooke avatar Jan 13 '23 20:01 EdwardCooke