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

For discussion purposes: #238

Closed
wants to merge 1 commit into from

Conversation

steinbitglis
Copy link
Contributor

These are some of the changes for the Unity version of the library.
ExampleRunner.cs has been added to enable running of the examples.
'ValidatingDuringDeserialization.cs' has been skipped since I didn't find out how to address the dependencies.

Consider if you want to include any of these changes. I'm maintaining a difference between the code bases for Unity users, for convenience, but of course I want to keep it as small as possible. In my package I don't want to require the 'UNITY' define flag either, so the package has a few additional changes compared to this.

These are some of the changes for the Unity version of the library.
ExampleRunner.cs has been added to enable running of the examples.
'ValidatingDuringDeserialization.cs' has been skipped, since the dependencies were non-trivial to add.
@aaubry
Copy link
Owner

aaubry commented Feb 3, 2017

Thanks, I'll try to merge them as best as possible. Ideally, you should not need to maintain different code for unity. I'll look at this during this week-end.

@aaubry
Copy link
Owner

aaubry commented Feb 6, 2017

Removing the dependency from xunit is a problem for the samples, because I use xunit to execute them. I could use something different, but I already have stuff built on top of that.

It seems that Unity specifies a set of #defines. Maybe we could take advantage of that? For example, if the unity-specific tweaks are wrapped inside #ifdef UNITY_5, it should be easier to merge these changes in the main code. What do you think?

@aaubry
Copy link
Owner

aaubry commented Feb 6, 2017

I've merged the "easy" changes, that don't cause conflicts, to let us focus on the hard ones.

@@ -106,6 +106,13 @@ public YamlMappingNode()
/// <summary>
/// Initializes a new instance of the <see cref="YamlMappingNode"/> class.
/// </summary>
public YamlMappingNode(int dummy) // dummy makes the call to the constructor unambiguous to buggy compilers like mono in Unity.
Copy link
Owner

Choose a reason for hiding this comment

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

Does the ambiguity arises from the constructors that take params arrays ?
If so, a workaround could be to change their signatures to:

public YamlMappingNode(
    KeyValuePair<YamlNode, YamlNode> firstChild,
    params KeyValuePair<YamlNode, YamlNode>[] otherChildren
)

This would probably remove the ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should catch 99.9% of the installations, I think,
#if UNITY_3_5 || UNITY_4_0 || UNITY_4_1 || UNITY_4_2 || UNITY_4_3 || UNITY_4_5 || UNITY_4_6 || UNITY_4_7 || UNITY_5

It's big, and it's not future proof, so that's why I edited the code maunally. Maybe I could just write a tool to do it for me or something.

I don't know how far back it actually works though, but we used YamlDotNet way back in Unity 3. YamlDotNet had to be fixed a lot before we could use it etc. but that's another old story.

I wrote my own loop to execute the tests from within Unity, but yes, we could use the defines to use both approaches.

The following seemed to resolve the ambiguity, as you predicted.
0001-Added-suggested-change-to-YamlMappingNode-legacy-com.patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here'se ExampleRunner.cs, that I'm using to expose the examples in the unity package, not depending on xunit. (Sorry about the Unity specific stuff mixed in there.)

ExampleRunner.cs
SampleAttribute.cs
TestOutputHelpersExtensions.cs

@EdwardCooke
Copy link
Collaborator

This PR appears to be abandoned, it's nearly 5 years old. I'm going to close it.

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.

None yet

3 participants