Skip to content

Fixes #108, #111, #99 #120

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

Closed
wants to merge 7 commits into from
Closed

Fixes #108, #111, #99 #120

wants to merge 7 commits into from

Conversation

MV10
Copy link
Contributor

@MV10 MV10 commented Apr 19, 2018

This PR should not be merged until PR 100 in Serilog.Settings.Configuration is merged and available on NuGet. The configuration changes described in the README will not work until the config package is updated.

Fixes #108, adds .NET Standard MS Configuration 2.x package support, converts the project to multi-target, and adds ColumnOptions configuration via config section (.NET Standard only).

Aso fixes #111 and fixes #99.

  • csproj altered for multi-targeting
  • BuildTarget folder created with net452 and netstandard2_0 sub-folders
  • config-related source moved to build-specific folders
  • unused/unneeded csproj constants (NETCORE, NETSTANDARD, NETSTANDARD2_0) removed
  • package reference tags split between a shared ItemGroup and TFM-conditional ItemGroups
  • moved SQL/.NET data type conversion to ConvertSqlDataType to simplify config extension methods
  • README updated
  • added optional IConfiguration appConfiguration to extension (for named connection string)
  • added optional IConfigurationSection columnOptionsSection parameter on config extension
  • miscellaneous cleanup (zombie using statements, formatting, etc.)

Update:

  • unit test csproj altered to support build-target-specific tests
  • added netstandard2 unit test for named connection string from IConfiguration
  • added netstandard2 unit test for ColumnOptions from IConfigurationSection

…d tests for config extension arguments specific to netstandard2
@MV10 MV10 changed the title Addresses #108 Fixes #108 Apr 20, 2018
@MV10 MV10 changed the title Fixes #108 Fixes #108, #111, #99 Apr 20, 2018
@mivano
Copy link
Contributor

mivano commented Apr 21, 2018

Thanks for the PR. Waiting for the other PR first. Ping me when ready.

@MV10
Copy link
Contributor Author

MV10 commented Apr 22, 2018

Looking at the conflicts, now I really wish I'd waited on those other two PRs before I started this one!

Anyway, the Serilog.Settings.Configuration PR has been merged, but it still needs to be available in NuGet. I'm not really sure how that is decided or handled, I emailed Nick to ask.

@MV10
Copy link
Contributor Author

MV10 commented Apr 22, 2018

Yeah, not sure how this PR will merge given my PR makes structural project changes (not just adding/removing lines). Anyway, notes for the latest push which does completely incorporate PR 118:

  • added audit config to .net452 and netstandard2_0 extensions
  • added MSSqlServerSinkTraits
  • added MSSqlServerAuditSink
  • modified MSSqlServerSink to match PR 118
  • updated packages.config
  • modified tests to match PR 118
  • removed unnecessary config section name constant my PR had added earlier
  • MSSqlServerSink vs PR 118: removed a few odd/unnecessary items like using System.Drawing

Builds, tests, and runs just fine, but I'm half wondering if it'll be easier (in terms of github conflict-detection freak-outs) to cancel this PR, fork again, and reapply my changes.

@MV10
Copy link
Contributor Author

MV10 commented Apr 29, 2018

@nblumhardt @mivano I'm going to cancel this and open a new one when PR 117 is resolved and merged. Since this PR involves large project-structure changes, it'll be faster and easier to deal with PR 118's changes starting from a fresh fork. I can re-apply my changes pretty quickly, maybe an hour or so.

Meanwhile I wanted to solicit opinions on a small change. This PR uses folders named BuildTargets/net451 and BuildTargets/netstandard2_0 for the old static ConfigurationManager-based code and the new IConfiguration code respectively. I don't like having versions baked into things like folders or filenames unless absolutely necessary.

My first thought was BuildTargets/netframework and BuildTargets/netstandard but newer .NET Framework versions are .NET Standard-compliant, so I'm concerned that might be confusing.

My next thought was BuildTargets/ConfigurationManager and BuildTargets/IConfiguration. It may still be confusing because ConfigurationManager is compatible with .NET Framework versions newer than the net451 build-target, but I think this expresses the intent better.

But the idea I like best is to treat ConfigurationManager as a special case for backwards-compatibility, and treat IConfiguration as the normal default folder structure. We know ConfigurationManager will eventually go away, and thanks to .NET Standard and the extensibility of the new model, I expect IConfiguration will be the norm for many years to come. So, perhaps Compatibility/ConfigurationManager and then just Configuration at the top-level for IConfiguration?

I'm open to suggestions.

@MV10
Copy link
Contributor Author

MV10 commented May 8, 2018

Will re-open shortly based on latest merges.

@MV10 MV10 closed this May 8, 2018
@MV10 MV10 deleted the ms_config_v2 branch May 8, 2018 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants