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

Scoped value formatters #2478

Open
FLAMESpl opened this issue Nov 24, 2023 · 14 comments
Open

Scoped value formatters #2478

FLAMESpl opened this issue Nov 24, 2023 · 14 comments
Assignees
Labels
api-approved API was approved, it can be implemented enhancement

Comments

@FLAMESpl
Copy link

FLAMESpl commented Nov 24, 2023

Background and motivation

This feature is about allowing adding custom IValueFormatters to AssertionScope. In my development I discovered that I need to selectively customize formatting of an object based on the test's context. There is an option to remove once added formatter but it will not play nicely if tests would be executed simultanously. Having scoped formatters have a side bonus of reusable parametrizing formatter instances.

API Proposal

First make IFormatterCollection interface.

public interface IFormatterCollection : ICollection<IValueFormatter>
{
}

Then add it to AssertionScope:

public class AssertionScope
{
    public IFormatterCollection Formatters { get; } = // initialize from parent scope
}

Formatter API remains unchanged. Internally it will utilize IFormatterCollection for sake of coherence.

API Usage

using (var scope = new AssertionScope())
{
   scope.Formatters .Add(new MyCustomFormatter())
   // some assertions
}

Alternative Designs

Alternative would be to add a custom key-value store in AssertionScope that could be read inside a IValueFormatter.

Risks

None I can think of.

Are you willing to help with a proof-of-concept (as PR in that or a separate repo) first and as pull-request later on?

Yes, please assign this issue to me.

@FLAMESpl FLAMESpl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 24, 2023
@dennisdoomen
Copy link
Member

Maybe scope.UsingFormatter(new MyCustomFormatter()) is nicer?

@FLAMESpl
Copy link
Author

It is also an option, this way collection of formatters could be internal.

@dennisdoomen
Copy link
Member

I'm fine with that. What about you @jnyrup ?

@jnyrup
Copy link
Member

jnyrup commented Feb 24, 2024

I like the proposal of having scoped formatters 👍

As the signature is Formatter.ToString(object value, FormattingOptions options = null) placing the scoped formatters on FormattingOptions might be a good place?

When retrieving all formatters in Formatter.Format it will then search in scoped custom formatters, static custom formatters and lastly static default formatters.

Perhaps we should also move the current Formatter.CustomFormatters to FormattingOptions?
This should be doable without introducing any breaking changes, by forwarding the existing AddFormatter and RemoveFormatter.
The statically set custom formatters will then be set on the static AssertionOptions.FormattingOptions and the scoped ones will be set on AssertionScope.FormattingOptions.

To set a scoped formatter you'll only have to do

using var scope = new AssertionScope();
scope.FormattingOptions.AddFormatter(new MyCustomFormatter());

@dennisdoomen
Copy link
Member

That sounds like a great idea. Let's go for that.

@dennisdoomen dennisdoomen added api-approved API was approved, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 24, 2024
@dennisdoomen dennisdoomen changed the title [API Proposal]: Scoped value formatters Scoped value formatters Feb 24, 2024
@dennisdoomen
Copy link
Member

@FLAMESpl are you still interested in this one?

@ChristoWolf
Copy link

@dennisdoomen: I would certainly be interested 😃

@dennisdoomen
Copy link
Member

I meant in implementing this.

@ChristoWolf
Copy link

Ah whoops, I misunderstood.

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Mar 21, 2024

I am willing to take that, if @FLAMESpl doesn't want to.

@ITaluone
Copy link
Contributor

Should a nested AssertionScope pick up all previous defined scoped formatters?

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 22, 2024

Everything that is associated with an AssertionScope flows in the nested scopes.

@ITaluone
Copy link
Contributor

Ok, the tricky part is how to determine which scoped formatters to remove at scope dispose 🤔

@dennisdoomen
Copy link
Member

Yeah, it means that the scope needs some state on which scope has added a particular formatter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved, it can be implemented enhancement
Projects
None yet
Development

No branches or pull requests

6 participants