Introduce support for deserializing via annotated constructor

[Arakade]

Introduce support for deserializing via annotated constructor call to allow read-only fields and properties.

Annotate a constructor with the new YamlConstructorAttribute and it will be used in preference to property and field setting.

Unit test added. It's largely copied from ObjectNodeDeserializer plus some tests to prove certain desirable behaviours. It's showing 81% coverage of the new code AnnotatedConstructorObjectNodeDeserializer. Missed lines are all corner cases (errors, the IValuePromise bits and non-use code-paths).

[aaubry]

Hi! Thanks for this. I did some experiments and overall it's working quite well.

I found some problems, though:

• When there is only one constructor on the class, I think the YamlConstructor attribute should not be necessary. This would reduce the friction and make the feature easier to use.
• This solution does not allow to assign properties in addition to calling the constructor:

  [Fact]
  public void Deserialize_using_constructor_allows_properties()
  {
      var yaml = Yaml.Text(@"
          readOnly: from constructor
          readWrite: from property
      ");
      var sut = new DeserializerBuilder()
          .WithNamingConvention(CamelCaseNamingConvention.Instance)
          .Build();
      var result = sut.Deserialize<TypeWithConstructorAndProperties>(yaml);
      Assert.Equal("from constructor", result.ReadOnly);
      Assert.Equal("from property", result.ReadWrite);
  }
  
  class TypeWithConstructorAndProperties
  {
      [YamlConstructor]
      public TypeWithConstructorAndProperties(string readOnly)
      {
          ReadOnly = readOnly;
      }
      public string ReadOnly { get; }
      public string? ReadWrite { get; set; }
  }
  

• It doesn't play well with naming conventions:

  [Fact]
  public void Deserialize_using_constructor_allows_naming_conventions()
  {
      var yaml = Yaml.Text(@"
          name: Jack
          moment_of_birth: 1983-04-21T20:21:03.0041599Z
          cars:
          - name: Mercedes
            year: 2018
      ");
  
      var sut = new DeserializerBuilder()
          .WithNamingConvention(UnderscoredNamingConvention.Instance)
          .WithTypeMapping<ICar, Car>()
          .Build();
  
      var person = sut.Deserialize<Person>(yaml);
      person.Name.Should().Be("Jack");
      person.MomentOfBirth.Kind.Should().Be(DateTimeKind.Utc);
      person.MomentOfBirth.ToUniversalTime().Year.Should().Be(1983);
      person.MomentOfBirth.ToUniversalTime().Month.Should().Be(4);
      person.MomentOfBirth.ToUniversalTime().Day.Should().Be(21);
      person.MomentOfBirth.ToUniversalTime().Hour.Should().Be(20);
      person.MomentOfBirth.ToUniversalTime().Minute.Should().Be(21);
      person.MomentOfBirth.ToUniversalTime().Second.Should().Be(3);
  }
  

I also tested anchors and even indirect circular references, which work correctly.

Let me know what are your thoughts on the above. Thanks

[Arakade]

Hi! Thanks for this. I did some experiments and overall it's working quite well.

Great!

I'll reply to each inline but the summary would be: I agree on all points and propose we add the functionality as is for now then extend it with the extra bits later.

* When there is only one constructor on the class, I think the `YamlConstructor` attribute should not be necessary. This would reduce the friction and make the feature easier to use.

Agreed. How would you prefer we determine the need to use the constructor (rather than ObjectNodeDeserializer)? Perhaps: If a sole constructor exists and more than zero arguments?

* This solution does not allow to assign properties in addition to calling the constructor:

Funny you should mention that, I was thinking exactly the same right after submitting my PR! 😁 My use cases so far have all been read-only objects.

Would you prefer adding the needed functionality to the new AnnotatedConstructorObjectNodeDeserializer, probably duplicating it straight from ObjectNodeDeserializer or... ?

* It doesn't play well with naming conventions:

Fair. I figured the best way to address would be a PropertyDescriptor (IIRC) that wraps a constructor argument then duplicate similar code from the ObjectNodeDeserializer. Sound reasonable?

I also tested anchors and even indirect circular references, which work correctly.

Ooh! Great. I hadn't looked at either of those so that's great to hear!

With my local copy sufficing for my needs for now, I've moved away from the YamlDotNet source for now. For this reason I worry that if we block integration of the functionality as is, it just delays its use by others. Depending on workload and complexity of solutions proposed, I'm happy to add the extra bits above but I hesitate to commit to timeline above "the next few months". Perhaps create GH "Issue" for all/each and assign to me and I'll pick them off as I go (or on-demand if users vote their desire)?

Thanks for such a useful library btw! We're prototyping using it as persistence format for an indie game we're developing. TTYS.

[Arakade]

Doing a little more work on this. Updates inline.

* When there is only one constructor on the class, I think the `YamlConstructor` attribute should not be necessary. This would reduce the friction and make the feature easier to use.

Agreed. How would you prefer we determine the need to use the constructor (rather than ObjectNodeDeserializer)? Perhaps: If a sole constructor exists and more than zero arguments?

I have this working locally now.

* This solution does not allow to assign properties in addition to calling the constructor:

Funny you should mention that, I was thinking exactly the same right after submitting my PR! 😁 My use cases so far have all been read-only objects.

Would you prefer adding the needed functionality to the new AnnotatedConstructorObjectNodeDeserializer, probably duplicating it straight from ObjectNodeDeserializer or... ?

Working on this next.

* It doesn't play well with naming conventions:

Fair. I figured the best way to address would be a PropertyDescriptor (IIRC) that wraps a constructor argument then duplicate similar code from the ObjectNodeDeserializer. Sound reasonable?

Not yet looked at this. Might you accept an updated PR that contains the other two to start?

[EdwardCooke]

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