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

Add Catel.Data.IObjectAdapter with a Reflection and ExpressionTree implementation #1447

Merged
merged 11 commits into from Oct 22, 2019

Conversation

@GeertvanHorrik
Copy link
Member

GeertvanHorrik commented Oct 20, 2019

Description of Change

Add Catel.Data.IObjectAdapter.

Issues Resolved

API Changes

None

Platforms Affected

  • All

Behavioral Changes

None

Testing Procedure

PR Checklist

  • I have included examples or tests
  • I have updated the change log
  • I am listed in the CONTRIBUTORS file
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
@GeertvanHorrik GeertvanHorrik added this to the 5.12.0 milestone Oct 20, 2019
@GeertvanHorrik GeertvanHorrik self-assigned this Oct 20, 2019
Copy link

ghost left a comment

I noticed some nuanced behavior within your implementation that I wonder should be really done. I'm not strongly suggesting it be changed but I am recommending it just because ref and out often get confused as to how they really work for the uninitiated.

@@ -49,7 +49,8 @@ public void Initialize(IServiceLocator serviceLocator)
serviceLocator.RegisterType<IXmlSerializer, XmlSerializer>();
serviceLocator.RegisterType<IXmlNamespaceManager, XmlNamespaceManager>();
serviceLocator.RegisterType<ISerializationManager, SerializationManager>();
serviceLocator.RegisterType<IObjectAdapter, ObjectAdapter>();
serviceLocator.RegisterType<Catel.Runtime.Serialization.IObjectAdapter, Catel.Runtime.Serialization.ObjectAdapter>();
serviceLocator.RegisterType<Catel.Data.IObjectAdapter, Catel.Data.ReflectionObjectAdapter>();

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

For consistency, these should probably not be referencing by the full namespace, unless this is a transitional PR.

/// <param name="memberName">The member name.</param>
/// <param name="value">The member value to update.</param>
/// <returns><c>true</c> if the member was retrieved; otherwise <c>false</c>.</returns>
public virtual bool GetMemberValue<TValue>(object instance, string memberName, ref TValue value)

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

Why the use of ref for TValue instead of out? I'm not sure we should encourage the use of two-way behavior with this method as it could introduce confusion to the implementer if they don't understand the nuanced difference between the two.

Most methods implementing a TryGetValue pattern like this return a boolean state and (when supported) a nullable out variable.

It should be up to the implementing caller to decide whether or not the final object is updated based on the return value of GetMemberValue. It should be out TValue? value and you simply do this for any fallback behavior where not true and avoid the pre-initialization behavior of ref:

value = null;
return default;
/// <param name="memberName">The member name.</param>
/// <param name="value">The value which will be updated if the member was read.</param>
/// <returns><c>true</c> if the member value was retrieved; otherwise <c>false</c>.</returns>
protected virtual bool GetPropertyValue<TValue>(object instance, string memberName, ref TValue value)

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

Same as above, should be using out TValue? value instead of ref TValue out

/// <param name="memberName">The member name.</param>
/// <param name="value">The value which will be updated if the member was read.</param>
/// <returns><c>true</c> if the member value was retrieved; otherwise <c>false</c>.</returns>
protected virtual bool GetFieldValue<TValue>(object instance, string memberName, ref TValue value)

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

Same as above, should be using out TValue? value instead of ref TValue out

@@ -55,7 +55,7 @@ public class BinarySerializer : SerializerBase<BinarySerializationContextInfo>,
/// <param name="typeFactory">The type factory.</param>
/// <param name="objectAdapter">The object adapter.</param>
/// <exception cref="ArgumentNullException">The <paramref name="serializationManager" /> is <c>null</c>.</exception>
public BinarySerializer(ISerializationManager serializationManager, ITypeFactory typeFactory, IObjectAdapter objectAdapter)
public BinarySerializer(ISerializationManager serializationManager, ITypeFactory typeFactory, Catel.Runtime.Serialization.IObjectAdapter objectAdapter)

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

Full namespace qualification. Is this necessary or transitional?

@@ -70,7 +70,7 @@ private enum MemberType
/// <exception cref="ArgumentNullException">The <paramref name="dataContractSerializerFactory" /> is <c>null</c>.</exception>
/// <exception cref="ArgumentNullException">The <paramref name="xmlNamespaceManager" /> is <c>null</c>.</exception>
public XmlSerializer(ISerializationManager serializationManager, IDataContractSerializerFactory dataContractSerializerFactory,
IXmlNamespaceManager xmlNamespaceManager, ITypeFactory typeFactory, IObjectAdapter objectAdapter)
IXmlNamespaceManager xmlNamespaceManager, ITypeFactory typeFactory, Catel.Runtime.Serialization.IObjectAdapter objectAdapter)

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

Full namespace qualification. Is this necessary or transitional?

@@ -23,7 +23,7 @@ namespace Catel
public static class JsonExtensions
{
private static readonly ISerializationManager SerializationManager = ServiceLocator.Default.ResolveType<ISerializationManager>();
private static readonly IObjectAdapter ObjectAdapter = ServiceLocator.Default.ResolveType<IObjectAdapter>();
private static readonly Catel.Runtime.Serialization.IObjectAdapter ObjectAdapter = ServiceLocator.Default.ResolveType<Catel.Runtime.Serialization.IObjectAdapter>();

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2019

Full namespace qualification. Is this necessary or transitional?

@GeertvanHorrik GeertvanHorrik changed the title [WIP] Add Catel.Data.IObjectAdapter with a Reflection implementation [WIP] Add Catel.Data.IObjectAdapter with a Reflection and ExpressionTree implementation Oct 22, 2019
@GeertvanHorrik GeertvanHorrik changed the title [WIP] Add Catel.Data.IObjectAdapter with a Reflection and ExpressionTree implementation Add Catel.Data.IObjectAdapter with a Reflection and ExpressionTree implementation Oct 22, 2019
@GeertvanHorrik GeertvanHorrik merged commit a321250 into develop Oct 22, 2019
2 checks passed
2 checks passed
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@delete-merged-branch delete-merged-branch bot deleted the feature/Expression-trees-in-ObjectAdapter branch Oct 22, 2019
@lock

This comment has been minimized.

Copy link

lock bot commented Oct 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2019
@GeertvanHorrik GeertvanHorrik added feature and removed improvement labels Oct 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
1 participant
You can’t perform that action at this time.