Add async implementations to the HtmlFormatter. #376

clrudolphi · 2025-06-02T15:45:22Z

🤔 What's changed?

Added async method implementations.

⚡️ What's your motivation?

Reqnroll will want to consume the async implementations in its implementation of Messages.

🏷️ What kind of change is this?

🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
[X ] I've changed the behaviour of the code
- [ X] I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- [ X] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

gasparnagy

Do we need to maintain both the async and the sync methods? Would it be just enough to keep the async one?

gasparnagy · 2025-06-03T11:14:15Z

dotnet/Cucumber.HtmlFormatter/JsonInHtmlWriter.cs

    public override void Write(char[] value)
    {
        Write(value, 0, value.GetLength(0));
    }

+    public async Task WriteAsync(char[] value)


Based on the warning, this method should have been done anyway.

'JsonInHtmlWriter.WriteAsync(char[])' hides inherited member 'TextWriter.WriteAsync(char[])'. Use the new keyword if hiding was intended.

Sorry, I was not clear with this one. This "override" is not necessary. The method in the base class is implemented in the same way, I think. So delete this method?

mpkorstanje · 2025-06-03T11:32:57Z

Do we need to maintain both the async and the sync methods?

Your call. The only difference is the resulting version number. If sync is removed then it will be a major release, otherwise a minor.

clrudolphi · 2025-06-03T11:38:52Z

Do we need to maintain both the async and the sync methods? Would it be just enough to keep the async one?

I put the [Obsolete] marker on the sync constructors.
We can release this as a minor and come back later to remove them completely.

…set up the sync methods of the class.

gasparnagy

Some cosmetic things...

gasparnagy · 2025-06-18T07:49:04Z

dotnet/Cucumber.HtmlFormatter/JsonInHtmlWriter.cs

    public override void Write(char[] value)
    {
        Write(value, 0, value.GetLength(0));
    }

+    public async Task WriteAsync(char[] value)


Sorry, I was not clear with this one. This "override" is not necessary. The method in the base class is implemented in the same way, I think. So delete this method?

gasparnagy · 2025-06-18T07:56:01Z

dotnet/Cucumber.HtmlFormatterTest/MessagesToHtmlWriterTest.cs

+            string html = await RenderAsHtmlAsync();
+            Assert.IsTrue(html.Contains("window.CUCUMBER_MESSAGES = [];"));
+        }
+
        [TestMethod]
        public void ItThrowsWhenWritingAfterClose()


(GitHub didn't allow me to add this comment to line 69)

These tests (testing the sync) should be deleted or wrap the constructor call with pragmas to disable this warning explicitly, like

#pragma warning disable CS0618 // Type or member is obsolete MessagesToHtmlWriter messagesToHtmlWriter = new MessagesToHtmlWriter(bytes, serializer); #pragma warning restore CS0618 // Type or member is obsolete

Same for all other warnings in this file.

In general, the compilation should produce no warnings.

I've made both sets of changes per your requests.

clrudolphi · 2025-06-19T16:34:52Z

I think this is ready to be merged, but I don't know why the javascript test is failing. Can someone take a look at that?

CHANGELOG.md

mpkorstanje · 2025-06-19T17:09:46Z

I think this is ready to be merged, but I don't know why the javascript test is failing. Can someone take a look at that?

Looks like that was already fixed on main.

mpkorstanje

Looks good. But a few remarks. I'll fix these right now.

mpkorstanje · 2025-06-19T17:13:28Z

CHANGELOG.md

@@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]
+### Added


I've changed this to added. It doesn't look like anything was removed. Is that correct w.r.t the guidance on http://semver.org?

mpkorstanje · 2025-06-19T17:13:46Z

dotnet/Cucumber.HtmlFormatter/JsonInHtmlWriter.cs

    public override void Write(char[] value)
    {
        Write(value, 0, value.GetLength(0));
    }

+    //public async Task WriteAsync(char[] value)


Can be removed?

mpkorstanje · 2025-06-19T17:19:34Z

dotnet/Cucumber.HtmlFormatter/MessagesToHtmlWriter.cs

    public MessagesToHtmlWriter(Stream stream, Action<StreamWriter, Envelope> streamSerializer) : this(new StreamWriter(stream), streamSerializer)
    {
    }
+    public MessagesToHtmlWriter(Stream stream, Func<StreamWriter, Envelope, Task> asyncStreamSerializer) : this(new StreamWriter(stream), asyncStreamSerializer) { }
+
+    [Obsolete("Cucumber.HtmlFormatter moving to async only operations. Please use the MessagesToHtmlWriter(StreamWriter, Func<StreamWriter, Envelope, Task>) constructor", false)]


This looks like a deprecation. So this needs its own deprecated entry in the changelog. This will inform the decision for the next version number.

mpkorstanje · 2025-06-19T17:33:13Z

And as a mental note, when removing the deprecated methods, please also mark the JsonInHtmlWriter as internal. That makes it less ambiguous which parts of this library are the API.

Atleast if I understood this all correctly: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/accessibility-levels

clrudolphi · 2025-06-19T17:37:45Z

@mpkorstanje Thanks for taking care of these items!

clrudolphi added 2 commits June 2, 2025 10:41

Added async implementations to the HtmlFormatter.

40a43b9

Updated CHANGELOG.md

298dc15

clrudolphi requested a review from gasparnagy June 2, 2025 15:45

gasparnagy reviewed Jun 3, 2025

View reviewed changes

Marked as obsolete the constructors of MessagesToHtmlWriter that are …

db731be

…set up the sync methods of the class.

clrudolphi marked this pull request as ready for review June 18, 2025 01:57

clrudolphi requested a review from gasparnagy June 18, 2025 01:58

gasparnagy approved these changes Jun 18, 2025

View reviewed changes

Changes per @gasparnagy review comments.

5a4766f

mpkorstanje reviewed Jun 19, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mpkorstanje added 2 commits June 19, 2025 19:04

Merge branch 'main' into dotnet_async

1cd8889

Merge branch 'main' into dotnet_async

5e7c77a

Add PR reference

b30e88e

mpkorstanje requested changes Jun 19, 2025

View reviewed changes

mpkorstanje added 2 commits June 19, 2025 19:25

Remove unused code

e267276

Update CHANGELOG

c61a2c1

mpkorstanje merged commit a1b3995 into main Jun 19, 2025
1 check passed

mpkorstanje deleted the dotnet_async branch June 19, 2025 17:33

mpkorstanje changed the title ~~Added async implementations to the HtmlFormatter.~~ Add async implementations to the HtmlFormatter. Jun 19, 2025

Uh oh!

Add async implementations to the HtmlFormatter. #376

Add async implementations to the HtmlFormatter. #376

Uh oh!

Conversation

clrudolphi commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

Uh oh!

gasparnagy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpkorstanje commented Jun 3, 2025

Uh oh!

clrudolphi commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gasparnagy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clrudolphi commented Jun 19, 2025

Uh oh!

Uh oh!

mpkorstanje commented Jun 19, 2025

Uh oh!

mpkorstanje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpkorstanje commented Jun 19, 2025

Uh oh!

Uh oh!

clrudolphi commented Jun 19, 2025

Uh oh!

Uh oh!

clrudolphi commented Jun 2, 2025 •

edited

Loading

clrudolphi commented Jun 3, 2025 •

edited

Loading