Improve formatting of methods with generics

[sander1095]

Hi!

I've written the following code which I find to be very readable:

[HttpGet("dosinglanes")]
public async Task<ActionResult<IReadOnlyCollection<GetProductListResponseModel>>> GetProductsOnDosingLanes()
{
    await Task.CompletedTask;
    return Ok();
}

However, csharpier turns this into the following:

[HttpGet("dosinglanes")]
public async Task<
    ActionResult<IReadOnlyCollection<GetProductListResponseModel[]>>
> GetProductsOnDosingLanes()
{
    await Task.CompletedTask;
    return Ok();
}

Which I find to be very unreadable. The > GetProductsOnDosingLanes() is a very.. unique way of formatting this code.

I would like Csharpier to keep the code as is. What are your thoughts on this?
I could simply add a // csharpier-ignore, but I don't think this code should be formatted :)

[sander1095]

I've implemented a temporary workaround by using .csharpierrc like this:

# Some long API method definitions are easier to read when on one line.
printWidth: 120

But this is not ideal for me, because the "default" print width is fine by me, except for this case.
I know formatting is a never-ending discussion, so feel free to close this issue if you believe this is issue is a matter of preference and the .csharpierrc is a good enough fix. Otherwise, let's have a discussion :D

Perhaps it would be possible to apply .csharpierrc rules for specific files/directories, including wildcards? so like /**/*Controller.cs would only have a longer printWidth.

[belav]

Perhaps it would be possible to apply .csharpierrc rules for specific files/directories, including wildcards? so like /**/*Controller.cs would only have a longer printWidth.

I believe you can make that work with .editorconfig. As of 0.26.0 csharpier supports reading editorconfig files.

I agree that the method isn't formatted very well. Maybe something like this?

[HttpGet("dosinglanes")]
public async 
    Task<ActionResult<IReadOnlyCollection<GetProductListResponseModel>>> 
    GetProductsOnDosingLanes()
{

Or if there is only a single generic parameter never break, which I believe would result in this no matter how long the method name gets

[HttpGet("dosinglanes")]
public async Task<ActionResult<IReadOnlyCollection<GetProductListResponseModel>>> GetProductsOnDosingLanes()
{

I'll have to play around a bit with other sample code to see how both of those end up, or if there are any other possiblities.

[jods4]

Current formatting in OP breaks the Rectangle rule, and it doesn't feel good in the middle of a generic type.

Maybe something like this?

public async 
    Task<ActionResult<IReadOnlyCollection<GetProductListResponseModel>>> 
    GetProductsOnDosingLanes()
{

Seems ok, altough maybe I'd keep the return type on first line:

public async Task<ActionResult<IReadOnlyCollection<GetProductListResponseModel>>> 
    GetProductsOnDosingLanes()
{

This made me think of a new concept that could solve many of csharpier not-so-pretty edge cases. It'll need more thinking and design but it has potential:
CSharpier should avoid breaks that create widows or orphans.

I borrow these terms from typography, what I mean for code is: lines that are very short, so the break doesn't actually save much.

In this example, leaving public async alone on a line is only 12 characters. Maybe that's not worthy of breaking and the return type should be left on the same line.
A similar argument could be made for the method name (so keep everything on one line as you suggest), although 26 characters might be enough to justify the break.
As a counter-example, if the method was named Apply(), even if the line was over the limit it would be silly to break it on its own line.

This concept could help here, and many other instances.
This example from #672 has many lines with 3 or 5 characters:
(EDIT: I have reformatted that example with CSharpier 0.26 to match the current behavior)

class ClassName
{
    void MethodName()
    {
        var searchRequest = new SearchDescriptor<ElasticsearchProduct>().WithAggregations(
            o =>
                o.WithFilter(
                    key,
                    f =>
                        f.WithFilter(fd => filter)
                            .WithAggregations(
                                ad =>
                                    ad.WithTerm(
                                        key,
                                        cd =>
                                            cd.WithField(facetField).WithSize(MaximumCategoryFacets)
                                    )
                            )
                )
        );
    }
}

Let's use the same breaking logic, but avoid breaks that leaves less than 10 chars of syntax on a single line:

class ClassName
{
    void MethodName()
    {
        var searchRequest = new SearchDescriptor<ElasticsearchProduct>().WithAggregations(o =>
            o.WithFilter(
                key,
                f => f
                    .WithFilter(fd => filter)
                    .WithAggregations(ad => 
                        ad.WithTerm(
                            key, 
                            cd => cd.WithField(facetField).WithSize(MaximumCategoryFacets)
                        )
                    )
            )
        );
    }
}

I think it's an improvement.
Note that I choose not to apply a minimum length rule to one argument in a list of arguments or to one method call in a chain of calls. I think that would hurt readability.

[belav]

Let's use the same breaking logic, but avoid breaks that leaves less than 10 chars of syntax on a single line:

I'm really curious if there may be an easy way to implement this. Although it probably needs to be applied selectively. Some of the parameters in your example are less than 10 chars. But those are part of a group, where o => is not. Definitely something worth exploring more!

[vlflorian]

Has anyone found a workaround or fix for this?

Also encountering really weird method formatting here but can't find a config option to disable it.

public async Task<
        PagedResult<LegalProcedureReason, Embedded<LegalProcedureReason>>
    > GetLegalProcedureReasonsAsync(bool bypassCache, CancellationToken token)
    {

I'd rather see something like this:

    public async Task<PagedResult<LegalProcedureReason, Embedded<LegalProcedureReason>>>
	    GetLegalProcedureReasonsAsync(bool bypassCache, CancellationToken token)
    {

[belav]

Some related issues #450 #468

Another example of a method

static async Task<
	Results<Ok<PaginatedResponse<QuestionnaireDisplayModel>>, ErroredProblem>
> AllQuestionnairesAsync(
	[AsParameters] PaginationRequest<IQuestionnaireService> request,
	CancellationToken cancellationToken
)

FWIW this is how prettier deals with similar generics in TS. I haven't looked to see if they have any discussions on better ways to deal with it.

function AllQuestionnairesAsync(
  request: Task<
    Results<Ok<PaginatedResponse<QuestionnaireDisplayModel>>, ErroredProblem>
  >,
  cancellationToken: CancellationToken,
): Task<
  Results<Ok<PaginatedResponse<QuestionnaireDisplayModel>>, ErroredProblem>
> {
  return true;
}