-
Notifications
You must be signed in to change notification settings - Fork 129
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 custom types in arrays and custom collections #203
Conversation
add new test platforms and unify msbuild conditional properties minor code cleanup PackageIcon instead of PackageIconUrl fix for simplified syntax on netcore3.0 fix SinkWithConfigurationBindingArgument test (wrong argument type)
@nblumhardt Any progress on this? |
On the top of my review queue - sorry @sungam3r :-) |
src/Serilog.Settings.Configuration/Serilog.Settings.Configuration.csproj
Outdated
Show resolved
Hide resolved
src/Serilog.Settings.Configuration/Serilog.Settings.Configuration.csproj
Outdated
Show resolved
Hide resolved
@nblumhardt friendly bump |
Hi @sungam3r - I see a couple of comments above from @skomis-mm that could be addressed; I don't think changing TFMs or dependency versions is strictly necessary for the PR (or at least needs some discussion). Sergey is one of the primary authors of this package, so working with him to help spread the review effort around would be appreciated. |
@nblumhardt @skomis-mm I made the necessary changes. |
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.
Hi @sungam3r,
Looks good, thanks.
One thing is dealing with _configurationAssemblies
around and passing it back and forth. I think when ResolutionContext
was introduced it could be incapsulated there. But it wasn't. Probably worth to also refactor this so we don't need to handle it in ObjectArgumentValue
. Or in separate PR? What do you think?
|
Ok. Will leave it for a few days before merge if someone else want to check. |
When will release package be published on nuget? |
Also:
relates to #201