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]: Parsability of strings #2632

Open
WhatzGames opened this issue May 5, 2024 · 18 comments
Open

[API Proposal]: Parsability of strings #2632

WhatzGames opened this issue May 5, 2024 · 18 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation feature

Comments

@WhatzGames
Copy link

WhatzGames commented May 5, 2024

Background and motivation

In .NET 7 static abstracts Members for interfaces were introduced.
One of them is the IParsable<TSelf> interface.
Currently there is no direct support to test for parsability.
To see if something can be parsed, one would currently have to call Invoking(<call specific parse here>).Should().NotThrow(), which seems a bit clunky, adds more overhead then necessary and (depending on the targeted type) does not always provide a helpful and clear failure reasoning.
The use of Invoking also results in loosing the reference to the original string inside of the call-chain.

API Proposal

public class StringAssertions<TAssertions> : ReferenceTypeAssertions<string, TAssertions>
    where TAssertions : StringAssertions<TAssertions>
{
    public AndWhichConstraint<TAssertions,TTarget> BeParsableInto<TTarget>(IFormatProvider? formatProvider = null, string because = "", params object[] becauseArgs) where TTarget : IParsable<TTarget>;
    public AndConstraint<TAssertions> NotBeParsableInto<TTarget>(IFormatProvider? formatProvider = null, string because = "", params object[] becauseArgs) where TTarget : IParsable<TTarget>;
}

API Usage

var number = "1"
number.Should()
.BeParsableInto<int>()
.And
.NotBeParsableInto<bool>();

Alternative Designs

public AndWhichConstraint<TAssertions,TTarget> BeParsableInto<TTarget>(string because = "", params object[] becauseArgs) where TTarget : IParsable<TTarget>;
public AndWhichConstraint<TAssertions,TTarget> BeParsableInto<TTarget>(IFormatProvider? formatProvider, string because = "", params object[] becauseArgs) where TTarget : IParsable<TTarget>;

public AndConstraint<TAssertions> NotBeParsableInto<TTarget>(string because = "", params object[] becauseArgs) where TTarget : IParsable<TTarget>;
public AndConstraint<TAssertions> NotBeParsableInto<TTarget>(IFormatProvider? formatProvider, string because = "", params object[] becauseArgs) where TTarget : IParsable<TTarget>;

other names that came to me were:
(Not)ParseInto
(Not)BeParsableAs

Risks

None that I know of, unless adding .NET 7+ support itself should be considered as a risk-factor.

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.

Edit:

  • corrected copy&paste mistake in overload.
  • corrected some spelling mistakes
@WhatzGames WhatzGames added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 5, 2024
@dennisdoomen
Copy link
Member

I didn't even know about IParsable<TSelf>. @jnyrup @arocheleau what do you think?

@arocheleau
Copy link
Contributor

To be honest, I don't know what to think about it. From a user perspective, it makes sense to want to support it to have simpler tests and better and more meaningful failure messages. In the past, how did you decide what makes a good candidate? Is it simply a matter of what is popular, common or heavily use? Since it was added to .NET 7, is it still too soon to consider? My guess is that the IParsable<TSelf> interface will be use more and more overtime as people start or convert their projects to .NET 7+.

Assuming that a BeParsableInto API is being added, what would be the underlying steps? I guess the code would have to:

  1. Verify if the target type implements the IParsable<TSelf> interface.
  2. Invoke the Parse or TryParse static method from the interface.

Which one of the 2 static methods the code would have to invoke? Is it important? Do we have to offer a TryParseInto API as well to differentiate one call from another?

@dennisdoomen
Copy link
Member

We would have to add a target for net7.0.

Which one of the 2 static methods the code would have to invoke? Is it important? Do we have to offer a TryParseInto API as well to differentiate one call from another?

The point is to verify whether the type implements IParsable<TSelf correctly, so I guess we would need to call both.

@jnyrup
Copy link
Member

jnyrup commented May 15, 2024

In the past, how did you decide what makes a good candidate? Is it simply a matter of what is popular, common or heavily use?

A combination of multiple considerations.

  • Number of 👍 on the API proposal.
  • It should most likely not add new nuget dependencies to FA.
  • It should not be too niche - this is fluffy measure.

Since it was added to .NET 7, is it still too soon to consider?

Our users use Fluent Assertions even with previews of .NET, see e.g. #1047

Verify if the target type implements the IParsable<TSelf> interface.

That is done at compile-time via the suggested generic constraint where TTarget : IParsable<TTarget>

Which one of the 2 static methods the code would have to invoke? Is it important? Do we have to offer a TryParseInto API as well to differentiate one call from another?

I think we should assume that Parse and ´TryParse` are implemented similarly
E.g. #1141 wanted to add assertions for all ways to enumerate a sequence, which we found too tightly coupled to an implementation rather than the overall behavior.

I imagine the flow would be something like:

var succes = TTarget.TryParse(Subject, out var parsed);
if !succes
  throw failure

return new AndWhichConstraint(this, parsed)

@WhatzGames
Copy link
Author

I think we should assume that Parse and TryParse are implemented similarly

This is a logical expectation considering how the dotnet/runtime has implemented the interface themselves.
E.g. the DateOnly implementation of I(Span)Parsable<TSelf>.Parse basically just ends up calling TryParse, with the only difference being Parse throws on failure.
And a similar approach is taken for numeric types and URIs upcoming as well.

I imagine the flow would be something like:

var succes = TTarget.TryParse(Subject, out var parsed);
if !succes
 throw failure
return new AndWhichConstraint(this, parsed)

I pretty much had the same kind of flow in mind.

@dennisdoomen
Copy link
Member

dennisdoomen commented May 16, 2024

Since it was added to .NET 7, is it still too soon to consider?

.NET 7 is already out of support, so maybe it's time to target .NET 8 as well.

I think we should assume that Parse and TryParse are implemented similarly

They are two different methods, with potentially two different implementations. So that's why I was suggesting that a BeParsableInto method could execute two assertions, one on Parse and one on TryParse

@jnyrup
Copy link
Member

jnyrup commented May 17, 2024

They are two different methods, with potentially two different implementations. So that's why I was suggesting that a BeParsableInto method could execute two assertions, one on Parse and one on TryParse

I'm beginning to think we're moving to close to implementation details 🤔

E.g. If I wanted to test that both object.Equals(object) and IEquality<T>.Equals(T) were both properly implemented I would call them directly instead of relying on which one FA would use.

@dennisdoomen
Copy link
Member

That's not exactly the same here. We're talking about a single interface that has two methods. If BeParsable calls Parse, you don't know if TryParse would work as well.

@jnyrup
Copy link
Member

jnyrup commented May 18, 2024

To me we must assume that Parse and TryParse can parse the exact set of strings.
The proposal seems not to be about verifying an implementation of IParsable<T>, but whether a string is parsable into T.

e.g.

string subject = MethodReturningStringifiedGuid();

subject.Should().BeParsableInto<Guid>();

@dennisdoomen
Copy link
Member

But what does it mean to be parsable to a GUID. Does it mean that you can call IParseable<TSelf>.Parse or TryParse?

@jnyrup
Copy link
Member

jnyrup commented May 21, 2024

But what does it mean to be parsable to a GUID. Does it mean that you can call IParseable<TSelf>.Parse or TryParse?

It doesn't matter, that's an implementation detail to whoever implemented IParseable<TSelf> on TSelf.

And as we don't know if the string is parsable to a Guid, we pick TryParse to avoid using exceptions for control flow.

@dennisdoomen
Copy link
Member

Then what is the point of this API. We could be using Parse, but then it may turn out that in production, TryParse fails. Or the other way around. Who are we to decide which one is the one we should use?

@WhatzGames
Copy link
Author

So if I understand you correctly, then your concern is about whether or not Parse and TryParse have a completely different implementation, separate from each other.

@jnyrup
Copy link
Member

jnyrup commented May 22, 2024

Then what is the point of this API.

As I understand it, the point of this API is given a correct implementation of IParseable<T> (implemented by myself or from e.g. the BCL) does my string parse to T.
For this purpose if we cannot use either of Parse and TryParse we can't use any of them.

If someone implements IParseable<MyOwn> and wants to test that their implementation is correct, they should in my opinion not use this API as they should not rely on whether we use Parse or TryParse.

@dennisdoomen
Copy link
Member

So if I understand you correctly, then your concern is about whether or not Parse and TryParse have a completely different implementation, separate from each othe

Yes. But with @jnyrup's response from 4 hours ago, I'm wondering what the goal of the API is. Is it to verify whether an implementation of IParsable is correct or if something is "just" parsable.

@WhatzGames
Copy link
Author

@jnyrup's explanation fits my intend with the proposal.
This is not to test whether my parser is correctly implemented, but rather if my string is a parsable value.
With this API I expect the implementation of IParsable to be done already

@arocheleau
Copy link
Contributor

arocheleau commented May 22, 2024

I have been thinking about it too. I came back to the basics and thought about the purpose of Fluent Assertions and the goal of this proposal. Fluent Assertions has better failing messages and helps the developer by giving more contextual informations about a failing test.

If we replace the IParseable<T> interface with IList<T>, I doubt we would be arguing about verifying if the developer implemented the IList<T> properly. That's not what Fluent Assertions is for. We could say in some way that it brings tools, but its up to the developer to use these tools and to make the tests to assert that its implementation of IList<T> or IParseable<T> works as expected. In case of failure, we only want to provide better contextual information.

We could argue about whatever it should use Parse or TryParse internally, but I guess this would be arguing about a possible default behavior of a feature. Here, the feature or the question we are trying to answer is whatever or not a string or input is parseable into another type, that's it.

In case of failure, I guess we would like to have a message that says something like: "Failed to parse input 'blah blah blah' to type XYZ because ...". Should we care about IParseable<T> at all? Maybe BeParsableInto<TTarget> should require a Func<> to handle the parsing. Given an input of type TInput (eg. string) and options (type to determine), we want this function to return 2 pieces of information, whatever the input was parsed successfully and if so the parsed result. It would be up to the developer to provide the function that actually invoke Parse or TryParse if it wants to test IParsable<T>.

public AndWhichConstraint<TAssertions,TTarget> BeParsableInto<TTarget>(Func<string, IFormatProvider?, Tuple<bool, TTarget?>> func, IFormatProvider? formatProvider = null, string because = "", params object[] becauseArgs);

In the previous example, the return type of the input function is Tuple<bool, TTarget?>. I think a better type should be used instead since there is only 2 possible outcome: true with a value of type TTarget or false with no value at all.

Here, I am probably getting to far into abstraction, but earlier I mention type TInput instead of string. I am wondering the difference about parsing versus converting. Overall, this looks similar to converting an input of type TInput into an output of type TOutput using some options/configurations (eg. IFormatProvider in case of IParseable<T>.Parse or IParseable<T>.TryParse, but it could be something else depending on what the developer function to be tested needs). Parsing and converting looks similar and would probably differ only in the failed output message. One would say "Failed to parse..." while the other would say "Failed to convert...".

@dennisdoomen
Copy link
Member

OK, I think that settles the discussion for me.

As far as I'm concerned, I think internally we call IParsable.Parse and if that throws an exception, we wrap that in a normal FA exception with either the message of the original exception or by passing it as an inner exception (whichever makes it easier to understand why it wasn't parsable).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation feature
Projects
None yet
Development

No branches or pull requests

4 participants