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

[API Proposal]: TextWriter.Aggregate #93623

Closed
stephentoub opened this issue Oct 17, 2023 · 18 comments · Fixed by #96732
Closed

[API Proposal]: TextWriter.Aggregate #93623

stephentoub opened this issue Oct 17, 2023 · 18 comments · Fixed by #96732
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Oct 17, 2023

UPDATED PROPOSAL 1/3/2024:

public abstract partial class TextWriter
{
+    public static TextWriter Aggregate(ReadOnlySpan<TextWriter> writers);
}

Open questions:

  • Is this the right name?
  • Should there be an optional parameter for leaveOpen?
  • Do we need an array-based overload? params?

Background and motivation

It's common in simple command-line apps/tools to want to write output to multiple places, e.g. to the console and to a file.

API Proposal

namespace System.IO

public sealed class TeeTextWriter : TextWriter
{
    public TeeTextWriter(params TextWriter[] writers);
    ... // overrides of all TextWriter methods
}

API Usage

using StreamWriter fileWriter = new StreamWriter("...");
var writer = new TeeTextWriter(fileWriter, Console.Out);
foreach (string s in strings)
    writer.WriteLine($"The value is {s}");

Alternative Designs

No response

Risks

No response

@stephentoub stephentoub added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO labels Oct 17, 2023
@stephentoub stephentoub added this to the 9.0.0 milestone Oct 17, 2023
@ghost
Copy link

ghost commented Oct 17, 2023

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

It's common simple console apps to want to write output to multiple places, e.g. to the console and to a file.

API Proposal

namespace System.Collections.Generic;

public sealed class TeeTextWriter : TextWriter
{
    public TeeTextWriter(params TextWriter[] writers);
    ... // overrides of all TextWriter methods
}

API Usage

using StreamWriter fileWriter = new StreamWriter("...");
var writer = new TeeTextWriter(fileWriter, Console.Out);
foreach (string s in strings)
    writer.WriteLine($"The value is {s}");

Alternative Designs

No response

Risks

No response

Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.IO

Milestone: 9.0.0

@adamsitnik
Copy link
Member

As a non-native speaker I had to check the dictionary to see what Tee means. Do you have any other ideas for the name?

@stephentoub
Copy link
Member Author

I was just basing on that on the Unix tee command. There are other options, though. Off the top of my head, MultiWriter, MultiTextWriter, PassthroughWriter, WrappingWriter, ... probably some others. For reference, there's also an open issue about a TeeReader.

@MichalPetryka
Copy link
Contributor

CombiningWriter, DelegatingWriter maybe?

@jkoritzinsky
Copy link
Member

We have a specialized version of this type in the test infrastructure here. I'd happily switch to an in-box version if this API were to be approved.

@ilmax
Copy link

ilmax commented Oct 17, 2023

CompositeTextWriter may be another viable name

@paulbartrum
Copy link

paulbartrum commented Oct 17, 2023

SplittingTextWriter? (The input text is split into two output streams.) Edit: Or DuplicatingTextWriter.

@Clockwork-Muse
Copy link
Contributor

... The most common use for this would normally be logging, though, I would imagine? But logging implementors already provides this behavior...

@stephentoub
Copy link
Member Author

... The most common use for this would normally be logging, though, I would imagine?

Not for me. For me it's usually that I'm writing a little tool that's generating something, and I want to save that out to a file, but I also want to see what it's generating, so I want a writer that writes both to a file and to the console. Basically the canonical use of tee.

@skyoxZ
Copy link
Contributor

skyoxZ commented Oct 18, 2023

I think it's unnecessary to make the class public. A static method in TextWriter is enough:

public abstract partial class TextWriter
{
    public static TextWriter Combine(params TextWriter[] writers);
}

@gfoidl
Copy link
Member

gfoidl commented Oct 19, 2023

The most common use for this would normally be logging, though

Imagine a CLI for calculations. You want output some stuff on the console, other stuff like results also on the console but in addition to a text-file. So plain tee won't work for this, as everything is written to the text-file.
With such a combined TextWriter you can be more specific what output goes where w/o manual doubling of the code.

@stephentoub
Copy link
Member Author

A static method

I'd be fine with that approach. In theory, a concrete type would be more flexible in that it would enable retrieving a list of wrapped writers and dynamically adding/removing writers, should that be desired, but I don't personally have a need for that.

@OKinane
Copy link

OKinane commented Dec 12, 2023

Wouldn't it be more generic to implement this combination logic at the stream level ?
It will be useful for example where we need to write bytes to a file and a socket at the same time.

@stephentoub stephentoub self-assigned this Dec 26, 2023
@stephentoub
Copy link
Member Author

Wouldn't it be more generic to implement this combination logic at the stream level ?

It might also be useful to have a Stream-based solution, but it wouldn't be instead of. It's very common to only have a TextWriter and not a Stream.

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 3, 2024
@stephentoub stephentoub changed the title [API Proposal]: TeeTextWriter [API Proposal]: TextWriter.Aggregate Jan 3, 2024
@skyoxZ
Copy link
Contributor

skyoxZ commented Jan 3, 2024

Should there be an optional parameter for leaveOpen?

How does it work when leaveOpen is true? I guess it means aggregateWriter.Dispose calls Flush and closes writing but doesn't dispose underlying writers. Do we need that?

Aggregate(ReadOnlySpan<TextWriter> writers) Do we need an array-based overload? params?

I think params is quite useful for this API.
Question: Is there some principles that why makes an API array-based only, ReadOnlySpan-based only, or both?

@stephentoub
Copy link
Member Author

I guess it means aggregateWriter.Dispose calls Flush and closes writing but doesn't dispose underlying writers. Do we need that?

Right. That's the question.

Is there some principles that why makes an API array-based only, ReadOnlySpan-based only, or both?

Whether the API is expected to be used in a language with limited span support, eg VB.

Up until now, params has also only been limited to arrays. But a) that's changing, and b) params is also of limited value now that we have collection expressions.

@bartonjs
Copy link
Member

bartonjs commented Jan 9, 2024

Video

  • We stuck with the static method alternative, but renamed Aggregate to CreateBroadcasting (during the meeting we'd discussed Broadcaster, but after subsequent discussion led to Broadcasting)
  • And converted it to a params-array method because of concern from non-C# callers.
public abstract partial class TextWriter
{
    public static TextWriter CreateBroadcasting(params TextWriter[] writers);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 9, 2024
@En3Tho
Copy link
Contributor

En3Tho commented Jan 9, 2024

@bartonjs If similar functionality for Streams land will it get the same / similar name? Will "Broadcaster" be a new general term for such functionality?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO
Projects
None yet
Development

Successfully merging a pull request may close this issue.