Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Upgrade/net.standard.2.0 #11
Conversation
|
@CraigSelbert thanks for your help on this! It appears that a lot of the changes are for white space. Is it possible your editor settings are different than those used by this repository? |
|
@mostlyjason I am using VS2017 version 15.3.2 with Resharper installed and the changes are all in the csproj files. To perform this work I had to create new projects and copy the files to the new location and during that process I do have VS reformat the code files for me. I did make one change to the On a side note I did spend some time refactoring the code, such as making public properties private readonly where applicable and various other coding style changes. I do think I will spend a few more hours cleaning up the code and would be happy for you to take a look if you are interested. |
|
I think if we are going to change the editor settings we should create a .editorconfig file. I believe resharper supports editor config? Also, I think we will have to test the code changes here to make sure they work across all versions including .NET framework 4. |
|
Eagerly awaiting this update via nuget - any word on how far away that might be? |
|
@mellevsen it might be a few weeks until our team has a chance to get to this one. If you have any extra time, we would love help with the editor config or testing this pull request. |
|
Just checking to see if this will be pulled in at some point? |
|
Hi @CraigSelbert, Thanks for your PR to support .NET Core 2.0 with Loggly. I want to let you know that I am working on to test your changes/PR on my local and I came to know that you have set the target framework- netcoreapp2.0;net452 which is minimum .NET Core 2.0 and .NET Framework 4.5.2(if I am not wrong?) at- https://github.com/CraigSelbert/log4net-loggly/blob/upgrade/net.standard.2.0/source/log4net-loggly/log4net-loggly.csproj#L5. If you see the Loggly document, you will notice that we have a support for .NET Framework 4 or higher and currently we have set the target framework to 4.0 at- https://github.com/loggly/log4net-loggly/blob/master/source/log4net-loggly/log4net-loggly.csproj#L13. I tried adding your code reference to my .NET Framework less than 4.5.2 and .NET Core version less than 2.0 console app and it was giving me compatibility errors. May I know if there is any specific reason to increase the minimum version support? If not, then can you please look into it to have the minimum support as before i.e. NET Framework 4 and should we not support .NET Core versions 1.0 and 1.1? Eagerly waiting for your response. Thanks! |
| @@ -109,7 +109,16 @@ private string GetMessageAndObjectInfo(LoggingEvent loggingEvent, out object obj | |||
| objInfo = null; | |||
| var bytesLengthAllowedToLoggly = EVENT_SIZE; | |||
|
|
|||
| if (loggingEvent.MessageObject != null) | |||
| if (!string.IsNullOrEmpty(loggingEvent.RenderedMessage)) | |||
psamit
Apr 12, 2018
Is this check specific for dot net core?
If yes then we should add a comment.
Is this check specific for dot net core?
If yes then we should add a comment.
Shwetajain148
Apr 12, 2018
I looked into the details and found the below for LogglyEvents.MessageObject property-
"Gets the message object used to initialize this event. Note that this event may not have a valid message object. If the event is serialized the message object will not be transferred. To get the text of the message the RenderedMessage property must be used not this property." from here.
I debugged the code here and could see that each time only the first if block was getting executed because we have the value in loggingEvent.RenderedMessage. The else block to check loggingEvent.MessageObject is not required anymore so I have removed it after ensuring.
And this is not specific to dot net core, it's a common use case for old .NET Frameworks too.
I looked into the details and found the below for LogglyEvents.MessageObject property-
"Gets the message object used to initialize this event. Note that this event may not have a valid message object. If the event is serialized the message object will not be transferred. To get the text of the message the RenderedMessage property must be used not this property." from here.
I debugged the code here and could see that each time only the first if block was getting executed because we have the value in loggingEvent.RenderedMessage. The else block to check loggingEvent.MessageObject is not required anymore so I have removed it after ensuring.
And this is not specific to dot net core, it's a common use case for old .NET Frameworks too.
|
LGTM |
|
Hi @CraigSelbert, @mellevsen and @noahwebster, I want to let you know that I have published a nuget beta package for .NET Core 2.0 support. Can you guys please have a look on it to test this out? You can install the beta package by following- (a) If you are using Visual Studio 2017 IDE then run the below command on Package Manager Console-
OR If you are using Visual Studio Code then run the below command on terminal-
You can also find the detailed instructions to setup and run the .NET Core 2.0 application on GitHub README file. See- https://github.com/loggly/log4net-loggly#net-core-support I'll wait for your feedback. Thanks! |
|
Thanks for this, I am sorry for not monitoring this pr, just a little busy |
Upgrade the code so it can work in a dot net core 2.0 application