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

Upgrade packages #46

Merged
merged 10 commits into from
Mar 22, 2025
Merged

Upgrade packages #46

merged 10 commits into from
Mar 22, 2025

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 6, 2025

Hello @pauldambra, as a first step in #45
I would like to do some house keeping.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 11, 2025

Hi @pauldambra, could I get a review here please?

@@ -2,10 +2,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp1.1</TargetFramework>
<TargetFramework>net9.0</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm so far outside of the dot net ecosystem...

is this a breaking change for folk using older versions of .Net?

(i.e. they won't be able to include it any more)

Copy link
Contributor

Choose a reason for hiding this comment

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

not against a version bump, just don't want to bump unnecessarily :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is not part of the actual shipped library, so it doesn't matter much. I chose the latest SDK since it's just a sample. I had to upgrade since netcoreapp1.1 has been deprecated for years now.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, it's the ponger 🙈

@@ -59,11 +51,11 @@ public void CanLogHelloWorld_WithDefaultFormatter()
[Fact]
public void CanLogHelloWorld_WithRawFormatter()
{
ConfigureTestLogger(new RawFormatter());
var arbitraryMessage = nameof(WhenLoggingViaTCP) + "CanLogHelloWorld_WithRawFormatter" + Guid.NewGuid();
ConfigureTestLogger(new CompactJsonFormatter());
Copy link
Contributor

Choose a reason for hiding this comment

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

same question (from ignorance on my part here) does this change the format of the message significantly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does change things a bit, but it still has no impact on the end-users. The RawFormatter is deprecated, so I replaced it. However, end-users can continue to use RawFormatter if they prefer. In this test case, the output is slightly different, but overall, I believe everything still works as expected.

@nojaf nojaf requested a review from pauldambra March 18, 2025 08:13
@nojaf
Copy link
Contributor Author

nojaf commented Mar 18, 2025

Hi @pauldambra, could you re-run that Windows build again?
I tried this a couple of times on my own Windows machine and the tests worked for me.

@pauldambra
Copy link
Contributor

consistently failing... i guess some setup problem 🤷

@nojaf
Copy link
Contributor Author

nojaf commented Mar 19, 2025

@pauldambra worked on my fork now:
image

Copy link
Contributor

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

👍

@pauldambra pauldambra merged commit e510a8c into serilog-contrib:master Mar 22, 2025
3 checks passed
@nojaf
Copy link
Contributor Author

nojaf commented Mar 22, 2025

Thanks for merging this @pauldambra.
What is the process of releasing new packages for this?
If possible, we would prefer a couple of (automated) releases so we can iterate on our progress in submitting our upstream fixes.

If there is no process, would something linked to a tag make sense to set up?
You create a new GitHub release, this makes a tag and there is a CI job that publishes to NuGet. Something along those lines?

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