Skip to content

Conversation

@odyth
Copy link

@odyth odyth commented Feb 13, 2016

instead of just throwing an exception in VerifyKeyUniqueness if two properties have the same name, lets first see if those objects inherit from one another. This will let you have a virtual property that is override and use the new keyword. We take the derived most class in this case and use that for serialization.

@yfakariya yfakariya added the bug Detected as bug label Mar 9, 2016
@yfakariya
Copy link
Member

Thank you for nice PR, but I've failed to merge because it looks that it does not solve hiding problem...

  1. Could you push or post test cases you used?
  2. Can I interpret this PR that it solves the problem the serializer ignore re-defined (hidden or marked with new) members in derived class? If so, VerifyKeyUniqueness is not suitable because it only handle members marked with custom attributes.

I tried to test your PR with following types:

    public class HidingBase
    {
        public string SameType { get; set; }
        public string ShouldBeInt32 { get; set; }
    }

    public class HidingBaseMarked
    {
        [MessagePackMember( 0 )]
        public string SameType { get; set; }

        [MessagePackMember( 1 )]
        public string ShouldBeInt32 { get; set; }
    }

    public class HidingNotMarked : HidingBase
    {
        public new string SameType { get; set; }
        public new int ShouldBeInt32 { get; set; }
    }

    public class HidingMarkedOnDerived : HidingBase
    {
        [MessagePackMember( 0 )]
        public new string SameType { get; set; }

        [MessagePackMember( 1 )]
        public new int ShouldBeInt32 { get; set; }
    }

    public class HidingMarkedOnBase : HidingBaseMarked
    {
        public new string SameType { get; set; }

        public new int ShouldBeInt32 { get; set; }
    }

    public class HidingMarkedOnBoth : HidingBaseMarked
    {
        [MessagePackMember( 0 )]
        public new string SameType { get; set; }

        [MessagePackMember( 1 )]
        public new int ShouldBeInt32 { get; set; }
    }

@yfakariya
Copy link
Member

This PR looks solve same as #161, so close this PR.
Thank you for great work and sorry for too-late response/discussion.

@yfakariya yfakariya closed this May 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Detected as bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants