Skip to content

Conversation

@ycherkes
Copy link

@ycherkes ycherkes commented Jul 15, 2023

Hi YAXLib team.

I want to use your fantastic library in my Object Dumper extension for Visual Studio.

Experimental version of extension

What is done in this PR:

Deprecation:

  • IMemberContext: PropertyInfo, FieldInfo, and MemberInfo properties marked as obsolete, added the MemberDescriptor property as a replacement for deprecated properties.

Improvements:

  1. Get rid of Lazy & ThreadLocal as they do impossible using this serializer in Visual Studio Debugger extension "The function evaluation requires all threads to run"

  2. Add the DoNotSerializeDefaultValues option to YAXSerializationOptions

  3. Add the TypeInspector property to SerializerOptions (allows type name and members customization without attributes)

  4. MemberWrapperCache and UdtWrapperCache: cache the type by type and SerializerOptions.

  5. Fixed failing tests: if choose Get_PooledObject and Parallel_Load_On_Specialized_Pools and run them only, the Parallel_Load_On_Specialized_Pools will fail.

image

ycherkes added 7 commits July 13, 2023 12:35
…is serializer in Visual Studio Debugger extension [ "The function evaluation requires all threads to run"](https://stackoverflow.com/questions/4460206/lazyt-the-function-evaluation-requires-all-threads-to-run)

2. Add the DontSerializeDefaultValues option to YAXSerializationOptions.
cache member type by type and SerializerOptions MemberWrapper<Type, IList<MemberWrapper> -> MemberWrapper<<(Type type, SerializerOptions options), IList<MemberWrapper>>
@axunonb axunonb marked this pull request as draft July 16, 2023 11:23
@ycherkes ycherkes changed the title Feature/yaxlib vs debuger friendly Feature/yaxlib visual studio debuger friendly Jul 16, 2023
@ycherkes ycherkes changed the title Feature/yaxlib visual studio debuger friendly Feature/yaxlib visual studio debugger friendly Jul 16, 2023
@ycherkes ycherkes marked this pull request as ready for review July 16, 2023 11:47
@ycherkes ycherkes marked this pull request as draft July 17, 2023 05:40
@ycherkes ycherkes marked this pull request as ready for review July 17, 2023 11:55
@axunonb axunonb self-requested a review July 17, 2023 14:37
@axunonb
Copy link
Member

axunonb commented Jul 17, 2023

Hi Yevhen

Thanks for the PR. It includes very nice improvements, while it's really huge and will require some time process.
For the moment, YAXLib team has only 1 member left ;-) so you're welcome to join.

Breaking changes

  • Yet I'm not aware of all, but the PR includes a couple of breaking changes. Please kindly name the breaking changes introduced in the PR description.
  • We had significant breaking changes over last months. So for the sake of other YAXLib users, we should be cautious. Is there a way to avoid or reduce them?

Few things that I came across when going throw the code for the first time:

  • SonarCloud Quality Gate fails for the PR. Please have a look at the Sonar Summary in terms of Maintainability and Coverage.
  • Well written XmlDoc is useful for users and maintainers. I was trying hard to get what we currently have, and it should not become less.

Naming conventions

  • Clearly, YAXLib does not follow the common naming convention for types. We should not mix Yax... and YAX... prefixes.
  • YAXLib 4.x still has few abbreviations used in member names. DoNot is preferred over Dont. In the past it was left as is to avoid a breaking change.

What do you think - does this make sense to you?

@ycherkes
Copy link
Author

ycherkes commented Jul 17, 2023

Hi @axunonb,
Thanks for the review.

I will try to fix the SonarCloud Quality Gate errors and fill out the XmlDoc as well.

I don't clearly understand what you mean when say "breaking changes" - all the functional tests left untouched. The only thing that could be impacted is Default cache sizes. I mean if it was previously cached by Type only but now caches by Type and SerializerOptions, that means that cache will be filled faster for use cases when serialization/deserialization occurs with multiple SerializerOptions.

I found the breaking changes

@ycherkes ycherkes marked this pull request as draft July 17, 2023 18:26
@axunonb
Copy link
Member

axunonb commented Jul 18, 2023

Hi Yevhen,
When looking at the implementation of ITypeResolver in the CustomTypeResolverTests I wonder what is the advantage of ITypeResolver over registering WellKnownTypes.Add(TheTypeYouWant) to control the serialization and deserialization. This gives you very fine grained control. Please have a look at the corresponding unit tests.
Also, when adding ITypeResolver to the SerializerOptions you just get 1 resolver per YAXSerializer instance, while you can add as many WellKnownTypes as you need. So I think ITypeResolvers would have to be registered similar to WellKnownTypes.

@ycherkes
Copy link
Author

ycherkes commented Jul 18, 2023

Hi @axunonb,
I know about WellKnownTypes and used it for the DateTimeOffset serialization.
But, unfortunately, it doesn't allow for skipping members at runtime
In opposition to WellKnownTypes, ITypeInfoResolver provides full control over serialization in runtime, not design time.
It can be a NamingPolicy or even customization of such built-in type as DateTime to write the value in ISO 8601 format, which can't be done by defining KnownType.

@axunonb
Copy link
Member

axunonb commented Jul 18, 2023

Thanks! WellKnownTypes can be added/removed at runtime. Skipping members at runtime is the missing feature, I agree, same as the NamingPolicy delegate. I like these two, but still would prefer to have these features in the known types instead of SerializerOptions, because they are actually type-specific. This should be possible.

As you mentioned built-in types like DateTime. You are right, this is an unnecessary limitation. Better move (part of) the logic in ReflectionUtils.ConvertBasicType(...) to the known types / known base types? DateTimeOffset and IpAddress should be a built-in known type, too, same as with DateOnly or TimeOnly which I added in v4.1.0.

@axunonb
Copy link
Member

axunonb commented Jul 20, 2023

Hi Yevhen,
As I can see you're still refactoring the YAXLib codebase. Having a glance at the 2 recent commits, I'd say you're introducing concepts to the better. For me it would be helpful to identify the target outcome of the PR more clearly, also having current users of YAXLib in mind. Maybe we could define a project which includes more steps to reach the goal, eventually publishing new versions on the way. Would be nice if you shared your thoughts.

@ycherkes
Copy link
Author

Guess, I've already finished this PR. The goal of the two last commits was to make the usage of ITypeInspector easy and do it similarly to Json.NET.ContractResolver or YamlDotNet.TypeInspectorSkeleton

@axunonb
Copy link
Member

axunonb commented Jul 20, 2023

@axunonb Can we add NetFramework 4.5 support to this PR?

Yevhen, I'm not happy going back to supporting Net Framework 4.5. Even .NetFramework 4.5.2, 4.6, 4.6.1 have reached end of support last year, and there has never been a request to target for .Net Framework 4.5.
As you suggest in the PR, the implication would also be, that not all features or types could not be supported when running under certain frameworks (i.e. DateOnly, TimeOnly). This makes maintenance and further development more complex. So although it's possible, disadvantages outweigh. I mean: we have NET8.0 ahead...

@ycherkes
Copy link
Author

OK. Not a problem. Agree with you.

/// <summary>
/// The member context interface provides information about the attributes of a member and member metadata.
/// </summary>
public interface IMemberContext
Copy link
Member

Choose a reason for hiding this comment

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

IMemberContext should be kept compatible with v4.1.0 so there's no need for a new major version

Copy link
Author

Choose a reason for hiding this comment

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

Done. Please take a look.

/// <summary>
/// The member's <see cref="MemberInfo" /> for member serialization, else <see langword="null" />.
/// </summary>
MemberInfo MemberInfo { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Keep, but mark as "Obsolete, will be removed in a future version"

Copy link
Author

Choose a reason for hiding this comment

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

Done. Please take a look.

/// The member's <see cref="PropertyInfo" /> for property serialization, else <see langword="null" />.
/// The member's <see cref="IMemberDescriptor" /> for member serialization, else <see langword="null" />.
/// </summary>
PropertyInfo? PropertyInfo { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Keep, but mark as "Obsolete, will be removed in a future version"

Copy link
Author

Choose a reason for hiding this comment

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

Done. Please take a look.

/// The member's <see cref="IMemberDescriptor" /> for member serialization, else <see langword="null" />.
/// </summary>
PropertyInfo? PropertyInfo { get; }
IMemberDescriptor MemberInfo { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename as MemberDescription

Copy link
Author

Choose a reason for hiding this comment

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

Done as MemberDescriptor. Please take a look.

Copy link
Member

@axunonb axunonb left a comment

Choose a reason for hiding this comment

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

Make the PR compatible with v4.1.0 to a avoid a new major version.

WrappedField.SetValue(obj, value);
}

public override string ToString()
Copy link
Member

@axunonb axunonb Jul 25, 2023

Choose a reason for hiding this comment

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

Shouldn't this be public override string? ToString()?
See https://sonarcloud.io/summary/new_code?id=axunonb_YAXLib&branch=pr%2F227
Maybe cover this method with unit test?

WrappedProperty.SetValue(obj, value, index);
}

public override string ToString()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be public override string? ToString()?
See https://sonarcloud.io/summary/new_code?id=axunonb_YAXLib&branch=pr%2F227
Maybe cover this method with unit test?

Copy link
Member

@axunonb axunonb left a comment

Choose a reason for hiding this comment

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

Excellent, thanks a lot!
We could merge this into a version/4.2.0 branch,
and then make DateTime a known type, and also add DateTimeOffset and IpAddress as known types as you suggested.
All together would then be the v4.2.0 release. Okay?

@axunonb axunonb changed the base branch from master to version/4.2 July 25, 2023 12:50
@axunonb axunonb merged commit 97d63f8 into YAXLib:version/4.2 Jul 25, 2023
@ycherkes
Copy link
Author

Yup, agree with you. I would also add the KeyValuePair to known types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants