Skip to content

fix Conversions setting to allow a local overide to specify Conversion.Default when the Global value is set to somthing else #813

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.

Sign up for GitHub

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

Closed
stocksr wants to merge 2 commits into from
Closed

fix Conversions setting to allow a local overide to specify Conversion.Default when the Global value is set to somthing else #813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
007952c
Add Failing Unit Tests
stocksr Jul 9, 2025
48d325b
Fix #810
stocksr Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Vogen.SharedTypes/Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Diagnostics;

namespace Vogen;

Expand All @@ -8,6 +9,11 @@ namespace Vogen;
[Flags]
public enum Conversions
{

[DebuggerBrowsable(DebuggerBrowsableState.Never)]
Unspecified = -1,


// Used with HasFlag, so needs to be 1, 2, 4 etc

/// <summary>
Expand Down
4 changes: 2 additions & 2 deletions src/Vogen.SharedTypes/ValueObjectAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class ValueObjectAttribute<T> : ValueObjectAttribute
/// </example>
/// </param>
public ValueObjectAttribute(
Conversions conversions = Conversions.Default,
Conversions conversions = Conversions.Unspecified,
Type? throws = null!,
Customizations customizations = Customizations.None,
DeserializationStrictness deserializationStrictness = DeserializationStrictness.AllowValidAndKnownInstances,
Expand Down Expand Up @@ -131,7 +131,7 @@ public class ValueObjectAttribute : Attribute
/// </param>
public ValueObjectAttribute(
Type? underlyingType = null!,
Conversions conversions = Conversions.Default,
Conversions conversions = Conversions.Unspecified,
Type? throws = null!,
Customizations customizations = Customizations.None,
DeserializationStrictness deserializationStrictness = DeserializationStrictness.AllowValidAndKnownInstances,
Expand Down
2 changes: 1 addition & 1 deletion src/Vogen.SharedTypes/VogenDefaultsAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class VogenDefaultsAttribute : Attribute
/// <param name="explicitlySpecifyTypeInValueObject">Every ValueObject attribute must explicitly specify the type of the primitive.</param>
public VogenDefaultsAttribute(
Type? underlyingType = null,
Conversions conversions = Conversions.Default,
Conversions conversions = Conversions.Unspecified,
Type? throws = null,
Customizations customizations = Customizations.None,
DeserializationStrictness deserializationStrictness = DeserializationStrictness.AllowValidAndKnownInstances,
Expand Down
2 changes: 1 addition & 1 deletion src/Vogen/BuildConfigurationFromAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ private BuildConfigurationFromAttributes(AttributeData att)
_matchingAttribute = att;
_invalidExceptionType = null;
_underlyingType = null;
_conversions = Conversions.Default;
_conversions = Conversions.Unspecified;
_customizations = Customizations.None;
_deserializationStrictness = DeserializationStrictness.Default;
_debuggerAttributes = DebuggerAttributeGeneration.Default;
Expand Down
6 changes: 3 additions & 3 deletions src/Vogen/CombineConfigurations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ public static VogenConfiguration CombineAndResolveAnythingUnspecified(
{
var conversions = (localValues.Conversions, globalValues?.Conversions) switch
{
(Conversions.Default, null) => VogenConfiguration.DefaultInstance.Conversions,
(Conversions.Default, Conversions.Default) => VogenConfiguration.DefaultInstance.Conversions,
(Conversions.Default, var globalDefault) => globalDefault.Value,
(Conversions.Unspecified, null) => VogenConfiguration.DefaultInstance.Conversions,
(Conversions.Unspecified, Conversions.Unspecified) => VogenConfiguration.DefaultInstance.Conversions,
(Conversions.Unspecified, var globalDefault) => globalDefault.Value,
(var specificValue, _) => specificValue
};

Expand Down
2 changes: 1 addition & 1 deletion src/Vogen/EnumExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ internal static class EnumExtensions
private static readonly int _maxCustomization = Enum.GetValues(typeof(Customizations)).Cast<int>().Max() * 2;
private static readonly int _maxDeserializationStrictness = Enum.GetValues(typeof(DeserializationStrictness)).Cast<int>().Max() * 2;

public static bool IsValidFlags(this Conversions value) => (int) value >= 0 && (int) value < _maxConversion;

public static bool IsValidFlags(this Conversions value) => (int) value >= -1 && (int) value < _maxConversion;

@SteveDunn SteveDunn Jul 18, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this should remain as >=0 as this check is done after config is merged. Nothing should remain unspecified after merging as that's when the defaults are applied. An unspecified value might mean we've forgotten to handle the default.

@stocksr stocksr Jul 18, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I changed this as without my change I was getting hundreds of VOG011 diagnostic warnings from the tests.

Is this the wrong place to fix that?

@SteveDunn SteveDunn Jul 19, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Oh, I see. When I get chance, I'll pull down this branch and take a look.

@SteveDunn SteveDunn Aug 1, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Apologies for the delay, I hope to have a bit of time at the weekend to catch up on things. Appreciate your PR!

@stocksr stocksr Aug 27, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👋 - any updates?

@SteveDunn SteveDunn Dec 18, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

So sorry for the extremely long delay! I'm on it now!

@SteveDunn SteveDunn Dec 19, 2025

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I checked out this PR and pushed, but for some reason, it created a new PR
#871

public static bool IsValidFlags(this Customizations value) => (int) value >= 0 && (int) value < _maxCustomization;

public static bool IsValidFlags(this DeserializationStrictness value) => (int) value >= 0 && (int) value < _maxDeserializationStrictness;
Expand Down
73 changes: 72 additions & 1 deletion tests/Vogen.Tests/ConfigurationTests/VogenConfigurationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public void Defaults()
{
var instance = VogenConfiguration.DefaultInstance;

instance.Conversions.Should().Be(Conversions.Default);
instance.Conversions.Should().Be(Conversions.TypeConverter | Conversions.SystemTextJson);
Comment thread
SteveDunn marked this conversation as resolved.
instance.Customizations.Should().Be(Customizations.None);
instance.DeserializationStrictness.Should().Be(DeserializationStrictness.Default);
instance.DebuggerAttributes.Should().Be(DebuggerAttributeGeneration.Full);
Expand Down Expand Up @@ -161,6 +161,77 @@ public void Local_beats_global_when_specified()
result.Conversions.Should().Be(Conversions.EfCoreValueConverter);
}

[Fact]
public void Local_beats_global_when_local_is_default_and_global_is_not()
{
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.SystemTextJson | Conversions.TypeConverter),
ConfigWithOmitConversionsAs(Conversions.NewtonsoftJson));

result.Conversions.Should().Be(Conversions.SystemTextJson | Conversions.TypeConverter);
}

[Fact]
public void Default_is_correct()
{
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.Default),
ConfigWithOmitConversionsAs(Conversions.Default));

result.Conversions.Should().Be(Conversions.SystemTextJson | Conversions.TypeConverter);
}

[Fact]
public void Unspecified_is_Default()
{
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.Unspecified),
ConfigWithOmitConversionsAs(Conversions.Unspecified));

result.Conversions.Should().Be(Conversions.Default);
}

[Fact]
public void Unspecified_is_overridable_locally()
{
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.Unspecified),
ConfigWithOmitConversionsAs(Conversions.NewtonsoftJson));

result.Conversions.Should().Be(Conversions.NewtonsoftJson);
}
[Fact]
public void Unspecified_is_overridable_globally()
{
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.NewtonsoftJson),
ConfigWithOmitConversionsAs(Conversions.Unspecified));

result.Conversions.Should().Be(Conversions.NewtonsoftJson);
}

[Fact]
public void Default_is_combinable_with_other_enum_members()
{
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.Default | Conversions.NewtonsoftJson),
ConfigWithOmitConversionsAs(Conversions.Unspecified));

result.Conversions.Should().Be(Conversions.NewtonsoftJson | Conversions.SystemTextJson | Conversions.TypeConverter);
}

[Fact]
public void Unspecified_when_combined_with_other_enum_members_forces_the_value_to_unspecified()
{
// This is a bit strange - but because of the -1 value of Unspecified - it works this way
// And I think that it is ok for this to be the behaviour
var result = CombineConfigurations.CombineAndResolveAnythingUnspecified(
ConfigWithOmitConversionsAs(Conversions.Unspecified | Conversions.NewtonsoftJson),
ConfigWithOmitConversionsAs(Conversions.Unspecified | Conversions.NewtonsoftJson));

result.Conversions.Should().Be(Conversions.Default);
}

private static VogenConfiguration ConfigWithOmitConversionsAs(Conversions conversions) =>
new(
null,
Expand Down
Loading