Skip to content

Conversation

tebeco
Copy link

@tebeco tebeco commented Jan 18, 2020

This PR follow the logic of both :
serilog/serilog-aspnetcore#165
serilog/serilog-extensions-hosting#17

What this PR change :

  • Add global.json
  • Add netcoreapp3.1 as a TargetFramework (Not the best way of resolution but it's still better than doing nothing)
  • Update AppVeyor image to Vs2019
  • Remove linter warning in powershell script
  • Use PackageIcon as PackageIconUrl is being deprecated
  • Always Resolve latest dependencies at build time for Serilog

Thoughts :

As said in this PR :
serilog/serilog-extensions-hosting#17

we could also just stop support for 2.x because 3.1 is LTS <=== This would require less complexity here, and customer using runtime/Microsoft.Exntesion.* would just need to pin this dependency to the current release
Note that the fact that a customer does not want to update it own dependency is driven to "pin down" it dependencies as they need evolve.
I have no idea What is Serilog stance on that but this is what Major is for ;)

For now i've tried to do this by using cross targeting, but as said the proper solution for this lib is to drop support for any Microsoft.Extensions.xxx 2.x as 3.1 is LTS since December 2019

Side note

I have no idea who use directly this dependency, but should it be concidered to just drop net46x and net45 ?
The same way Major version and pinning works, and also since net472/net48 are supposed to be netstandard2.0 they should show be able to resolves 2.1 dependencies

{
_configurationReader = new ConfigurationReader(
JsonStringConfigSource.LoadSection(@"{ 'Serilog': { } }", "Serilog"),
JsonStringConfigSource.LoadSection(@"{ ""Serilog"": { } }", "Serilog"),
Copy link
Contributor

@sungam3r sungam3r Jan 18, 2020

Choose a reason for hiding this comment

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

This may be helpful.

And then

  public override void Load()
            {
                Load(StringToStream(_json.ToValidJson()));
            }

Still waiting for merge :(

Copy link
Author

Choose a reason for hiding this comment

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

i did not saw that PR ;)
Also the double quote seems fine for all TFM, do we want to have different JSON delimiter since one works for all ?

Copy link
Contributor

Choose a reason for hiding this comment

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

My interest was to keep the number of changes in PR to a minimum. I achieved this with a few lines of code instead of a lot of refactoring. As a result, the tests work on both platforms. In addition, you will simply stop testing a single quote syntax. This is degradation of the test coverage.

@tebeco tebeco closed this Apr 20, 2020
@tebeco tebeco deleted the update-to-lts-31 branch April 20, 2020 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants