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

Add a extension .Subject() on Task<AndWhichConstraint<...>> to return Task<TMatchedElement> #1959

Open
jmg48 opened this issue Jul 21, 2022 · 7 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation

Comments

@jmg48
Copy link

jmg48 commented Jul 21, 2022

Add a extension .Subject() Task<AndWhichConstraint<...>> to return Task

If I have an async assertion such as .ShouldNotThrowAsync() and I want to access and store the subject of the assertion, I need to await the assertion in brackets and then access the .Subject, which can feel a little clumsy and hard to read with brackets extending across several lines.

Instead, if I had an extension method .Subject() on Task<AndWhichConstraint<TParentConstraint, TMatchedElement>> which returned Task<TMatchedElement> then I could access the subject fluently as a task and await that instead.

Hope this sounds useful - I'm happy to contribute this change if it meets with approval!

An assertion that I could write today:

var version = (await new HttpClient().Invoking(
        async httpClient => await httpClient.GetStringAsync("https://some.url/version"))
    .Should()
    .NotThrowAsync()).Subject;

(note the brackets enclosing four lines of code)

The extension method I propose adding:

public static class AssertionTaskExtensions
{
    public static async Task<TMatchedElement> Subject<TParentConstraint, TMatchedElement>(
        this Task<AndWhichConstraint<TParentConstraint, TMatchedElement>> assertions) => (await assertions).Subject;
}

The original assertion, rewritten to use this extension method:

var version = await new HttpClient().Invoking(
        async httpClient => await httpClient.GetStringAsync("https://some.url/version"))
    .Should()
    .NotThrowAsync().Subject();

(note the brackets are no longer needed)

@jnyrup jnyrup added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 21, 2022
@dennisdoomen
Copy link
Member

Maybe naming it AsSubject or ToSubject would better align with the .NET naming conventions?

@lg2de
Copy link
Contributor

lg2de commented Jul 23, 2022

The class AndWhichConstraint provides the context of the "result". In this context the property name Subject is correct, because it the "subject" of the "result context".

Focusing on the fluent statement we want to have "result" and not the "subject". (The subject here is IMHO the HttpClient.)

So, I would recommend ToResult().
What do you think?

@dennisdoomen
Copy link
Member

Yeah, I can live with ToResult() as well.

@jmg48
Copy link
Author

jmg48 commented Jul 23, 2022

This is a great question. I think given that the purpose of this method is to allow us to access something that can be awaited to get the .Subject of the assertion, I think the idiomatically correct naming (per other .NET APIs) would be .SubjectAsync()

Using Result in the method name would suggest a blocking call to access the result of a task, which is usually considered a bad thing in async programming, and is also not what this method would do.

Thanks for raising the question of naming - on reflection I do think .SubjectAsync() would be the best name for this method.

@jnyrup
Copy link
Member

jnyrup commented Jul 23, 2022

@jmg48 The .NET community is somewhat split these days on whether async methods can, should or must be suffixed with Async.

In my own tests these days, I tend to use NotThrow less and less, and write something like

// Arrange
...

// Act
var version = await new HttpClient().GetStringAsync("https://some.url/version");

// Assert
version.Should().Be("foo");

For inspiration, we do have support for writing

await new HttpClient().Invoking(httpClient => httpClient.GetStringAsync("https://some.url/version"))
    .Should().NotThrowAsync().WithResult("foo");

@lg2de
Copy link
Contributor

lg2de commented Jul 23, 2022

In FluentAssertions (FA) I (!!!) would like to omit the Async suffix because it breaks the "Fluentness".
(Today we have a mix because of the long history and possible breaking changes.)

Please note @jmg48, that "subject" is not clear enough. "Do I get the original subject or the subject of the result context?"
This is why I (!!!) think "result" is the better naming.

Because of this "conflict" I think whether its a good idea to make to subject that accessible.
The intention of FA is supporting assertions not the data itself. This may always create conflicts in naming.

@jmg48
Copy link
Author

jmg48 commented Jul 23, 2022 via email

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
Projects
None yet
Development

No branches or pull requests

4 participants