Skip to content
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

Update to lts 31 #205

Closed
wants to merge 10 commits into from
Closed

Update to lts 31 #205

wants to merge 10 commits into from

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

@@ -14,7 +14,7 @@ public class ConfigurationReaderTests
public ConfigurationReaderTests()
{
_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