-
Notifications
You must be signed in to change notification settings - Fork 794
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 support for multi-dimensional arrays #2035
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sending this. Keen to look at the IScalarConversionPolicy
route, since it would keep some complexity out of the (already a bit complex) PropertyValueConverter
class.
@@ -21,6 +21,7 @@ | |||
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.3" PrivateAssets="all" /> | |||
<PackageReference Include="xunit" Version="2.4.2" /> | |||
<PackageReference Include="System.ValueTuple" Version="4.5.0" /> | |||
<PackageReference Include="Shouldly" Version="4.1.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keen not to introduce a new inconsistent set of assertions into this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
evt.Properties.Count.ShouldBe(1); | ||
var arr = evt.Properties["Value"].ShouldBeOfType<SequenceValue>(); | ||
arr.Elements.Count.ShouldBe(6); | ||
arr.LiteralValue().ShouldBe("[a,b,c,d,e,f]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would expect [[a,b],[c,d],[e,f]]
here I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it intentionally since any array is just a sequence in terms of serilog regardless dimensions. Nevertheless I can change implementation to emit sequence of sequences if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -214,6 +214,29 @@ bool TryConvertEnumerable(object value, Type type, Destructuring destructuring, | |||
{ | |||
result = SequenceValue.Empty; | |||
} | |||
else if (list is Array a && a.Rank > 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be plugged in as an IScalarConversionPolicy
instead? Since those are checked before this code is reached, we could avoid the extra condition here. We do this already to specialize conversion of byte arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IScalarConversionPolicy
has no access to
- _depthLimiter (can be solved)
destructuring
boolean argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, we should not use IScalarConversionPolicy
here, because the determining factor whether or not using IScalarConversionPolicy
is the type of object inside the array, regardless of the dimensions of the array.
} | ||
else | ||
{ | ||
throw new NotSupportedException("Serilog does not support multi-dimensional arrays with Rank > 3."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is not a usage error but just a limitation, perhaps we should just use
result null;
return false;
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is a good candidate to get into 4.0; shall I make these final tweaks and merge this, @sungam3r?
fixes #2019