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

Avoid wrapping method arguments including a block-bodied lambda #1186

Open
jcracknell opened this issue Feb 21, 2024 · 4 comments
Open

Avoid wrapping method arguments including a block-bodied lambda #1186

jcracknell opened this issue Feb 21, 2024 · 4 comments

Comments

@jcracknell
Copy link

Input:

await SessionManager.Session(cancellationToken, async context =>
{
    var records = await context
        .DbContext.Entity.AsNoTracking()
        .Where(
            context.DbContext.PredicateFor(criteria).TranslatedBy(MkEntityPredicates)
        )
        .ToListAsync(context.CancellationToken);

    var entities = await MarshalEntities(records);

    return entities.ToImmutableDictionary(e => e.Id.Value);
});

Output:

await SessionManager.Session(
    cancellationToken,
    async context =>
    {
        var records = await context
            .DbContext.Entity.AsNoTracking()
            .Where(
                context.DbContext.PredicateFor(criteria).TranslatedBy(MkEntityPredicates)
            )
            .ToListAsync(context.CancellationToken);

        var entities = await MarshalEntities(records);

        return entities.ToImmutableDictionary(e => e.Id.Value);
    }
);

Expected behavior:

It would be nice if the formatter could avoid wrapping arguments to method calls where the call includes a block-bodied lambda on the assumption that the method call represents some form of flow control.

@belav
Copy link
Owner

belav commented Feb 23, 2024

FWIW prettier appears to avoid breaking arguments when one of them is a lambda. I haven't looked into the code to see if this was intenional or not, but I assume that it was.

Prettier output

await SessionManager.Session(cancellationToken, async (context) => {
  var records = await context.DbContext.Entity.AsNoTracking()
    .Where(
      context.DbContext.PredicateFor(criteria).TranslatedBy(MkEntityPredicates),
    )
    .ToListAsync(context.CancellationToken);

  var entities = await MarshalEntities(records);

  return entities.ToImmutableDictionary((e) => e.Id.Value);
});

@jcracknell
Copy link
Author

Sorry, to be clear this is using prettier with prettier-plugin-csharp?

I'd be fine with wrapping arguments when they contain a lambda which is not block-bodied, e.g.

var exception = Assert.Throws<Exception>(
    () => my.Incredibly.Long.Ass.Lambda.Expression
);

but blocks should probably be treated as blocks regardless of where they appear:

var exception = Assert.Throws<Exception>(() =>
{
    throw new Exception();
});

@Rudomitori
Copy link
Contributor

Some minimal API samples from the Microsoft's documentation

app.MapPost("/todoitems", async (Todo todo, TodoDb db) =>
{
    db.Todos.Add(todo);
    await db.SaveChangesAsync();

    return Results.Created($"/todoitems/{todo.Id}", todo);
});

app.MapPut("/todoitems/{id}", async (int id, Todo inputTodo, TodoDb db) =>
{
    var todo = await db.Todos.FindAsync(id);

    if (todo is null) return Results.NotFound();

    todo.Name = inputTodo.Name;
    todo.IsComplete = inputTodo.IsComplete;

    await db.SaveChangesAsync();

    return Results.NoContent();
});

app.MapDelete("/todoitems/{id}", async (int id, TodoDb db) =>
{
    if (await db.Todos.FindAsync(id) is Todo todo)
    {
        db.Todos.Remove(todo);
        await db.SaveChangesAsync();
        return Results.NoContent();
    }

    return Results.NotFound();
});

@rcdailey
Copy link

rcdailey commented Nov 21, 2024

This one especially irks me because it turns this:

    private static QualityProfileConfigYaml MergeQualityProfile(QualityProfileConfigYaml a, QualityProfileConfigYaml b)
    {
        return a with
        {
            Upgrade = Combine(a.Upgrade, b.Upgrade, (a1, b1) => a1 with
            {
                Allowed = b1.Allowed ?? a1.Allowed,
                UntilQuality = b1.UntilQuality ?? a1.UntilQuality,
                UntilScore = b1.UntilScore ?? a1.UntilScore
            }),
            MinFormatScore = b.MinFormatScore ?? a.MinFormatScore,
            QualitySort = b.QualitySort ?? a.QualitySort,
            ScoreSet = b.ScoreSet ?? a.ScoreSet,
            ResetUnmatchedScores = Combine(a.ResetUnmatchedScores, b.ResetUnmatchedScores, (a1, b1) => a1 with
            {
                Enabled = b1.Enabled ?? a1.Enabled,
                Except = Combine(a1.Except, b1.Except, (a2, b2) => Combine(a2, b2, (a3, b3) => a3
                    .Concat(b3)
                    .Distinct(StringComparer.InvariantCultureIgnoreCase)
                    .ToList()))
            }),
            Qualities = b.Qualities ?? a.Qualities
        };
    }

Into this:

    private static QualityProfileConfigYaml MergeQualityProfile(QualityProfileConfigYaml a, QualityProfileConfigYaml b)
    {
        return a with
        {
            Upgrade = Combine(
                a.Upgrade,
                b.Upgrade,
                (a1, b1) =>
                    a1 with
                    {
                        Allowed = b1.Allowed ?? a1.Allowed,
                        UntilQuality = b1.UntilQuality ?? a1.UntilQuality,
                        UntilScore = b1.UntilScore ?? a1.UntilScore,
                    }
            ),
            MinFormatScore = b.MinFormatScore ?? a.MinFormatScore,
            QualitySort = b.QualitySort ?? a.QualitySort,
            ScoreSet = b.ScoreSet ?? a.ScoreSet,
            ResetUnmatchedScores = Combine(
                a.ResetUnmatchedScores,
                b.ResetUnmatchedScores,
                (a1, b1) =>
                    a1 with
                    {
                        Enabled = b1.Enabled ?? a1.Enabled,
                        Except = Combine(
                            a1.Except,
                            b1.Except,
                            (a2, b2) =>
                                Combine(
                                    a2,
                                    b2,
                                    (a3, b3) =>
                                        a3.Concat(b3).Distinct(StringComparer.InvariantCultureIgnoreCase).ToList()
                                )
                        ),
                    }
            ),
            Qualities = b.Qualities ?? a.Qualities,
        };
    }

This one is a little different, but non destructive mutation of record types should fall into the same category, especially if it is done within an expression bodied lambda.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants