dart-lang / dart_style

An opinionated formatter/linter for Dart code
https://pub.dev/packages/dart_style
BSD 3-Clause "New" or "Revised" License
645 stars 117 forks source link

Not really happy with the indentation in a specific combination of language features #1463

Closed escamoteur closed 3 weeks ago

escamoteur commented 3 months ago

Sorry for this general title but it's best to see what I mean. I have this piece of code

grafik

adding a trailing comma behind cmdAddMessage doesn't make it much better

grafik

any idea how to improve the overall indentation

escamoteur commented 3 months ago

Actually if I replace the lambda with a tearoff it looks much better. Could that be possible with the lambda in place? grafik

srawlins commented 3 months ago

Thanks for filing an issue and for the screenshots. With text, others can play with the results. No one wants to type that in. Narrowing the text down to a minimal reproduction also helps others focus on what indentation is undesirable, and what is unrelated.

What indentation are you not happy with?

escamoteur commented 3 months ago

I fear that to get that sort of indentation I would need to provide a full project. But I m pretty sure @munificent will spot the problem immediately. If not I will try to create a self contained Dart file that reproduces this.

If you compare the last screenshot without the lamda it looks pretty ok. The other two versions don't indent the lambda block enough and the cascade operator indents even more. Am 6. Mai 2024, 19:59 +0200 schrieb Sam Rawlins @.***>:

Thanks for filing an issue and for the screenshots. With text, others can play with the results. No one wants to type that in. Narrowing the text down to a minimal reproduction also helps others focus on what indentation is undesirable, and what is unrelated. What indentation are you not happy with? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

munificent commented 3 months ago

I fear that to get that sort of indentation I would need to provide a full project.

All you need to include is the entire declaration, so just late final Command<MessageUpdateResult, void> addNewMessageCommand = ... up to the end of the field initializer expression. If you can add that to this issue, I'll take a look at how it gets formatted in the new formatter.

I agree the formatting looks wonky here. But also... you are doing a lot of work in that field initializer! 😅 It might be better to move that to a separate function or something. Even if the formatter does a better job, it's still going to be pretty hard to read.

munificent commented 3 weeks ago

Hey, @escamoteur, I'd be happy to try this out in the new formatter and see how it looks... but I'm definitely not going to retype all that code by hand. :) Can you include it as text in this issue? Thanks!

Juliotati commented 3 weeks ago

@munificent the code snippet below is from the last shared image xD

  late final Command<MessageUpdateRequest, void> addNewMessageCommand =
  Command.createAsyncNoResult<MessageUpdateRequest>(_addNewMessage,
      errorFilter: ApiErrorFilter(
        (error) => error.code == 412 || error.code == 403,
        ErrorReaction.localHandler,
      ),debugName: cmdAddMessage)
    ..errors.listen((ex,_) {
      final apiError ex!.error as ApiException;

    /// TODO adjust to chatmessagesource
    items.removeLast();
    lastMessage = null;
    refreshItemCOunt();
    if (apiError.message?.contains('limited to 15 people') ?? false) {
      di<InteractionManager>().pushToastWithBuilder(
        (context) => ToastConfig(
          context.l10n.messagingLimitPerDay,
        ),
      );
    } else if (apiError.message?.contains('Permission denied') ?? false) {
      di<InteractionManager>().pushToastWithBuilder(
        (context) => ToastConfig(
          context.l10n.defaultMessagingRestriction,
        ),
      );
    }
  });
munificent commented 3 weeks ago

Thanks, @Juliotati! I ran that code through the forthcoming tall style formatter and got:

  late final Command<MessageUpdateRequest, void> addNewMessageCommand =
      Command.createAsyncNoResult<MessageUpdateRequest>(
          _addNewMessage,
          errorFilter: ApiErrorFilter(
            (error) => error.code == 412 || error.code == 403,
            ErrorReaction.localHandler,
          ),
          debugName: cmdAddMessage,
        )
        ..errors.listen((ex, _) {
          final apiError = ex!.error as ApiException;

          /// TODO adjust to chatmessagesource
          items.removeLast();
          lastMessage = null;
          refreshItemCOunt();
          if (apiError.message?.contains('limited to 15 people') ?? false) {
            di<InteractionManager>().pushToastWithBuilder(
              (context) => ToastConfig(context.l10n.messagingLimitPerDay),
            );
          } else if (apiError.message?.contains('Permission denied') ?? false) {
            di<InteractionManager>().pushToastWithBuilder(
              (context) => ToastConfig(
                context.l10n.defaultMessagingRestriction,
              ),
            );
          }
        });

The initial snippet with the in place function expression looks like:

  late final Command<MessageUpdateRequest, void> addNewMessageCommand =
      Command.createAsyncNoResult<MessageUpdateRequest>(
          (newMessage) async {
            final chatApi = ChatsApi(di<ApiClient>());
            if (_target == null) {
              assert(_chatPartner != null);
              _target =
                  await chatApi.sendMessageToUserAndCreateChatIfNeeded(
                    _chatPartner!.id.toString(),
                    messageUpdateRequest: newMessage,
                  );
            } else {
              await chatApi.addMessageToChat(
                chatId,
                messageUpdateRequest: newMessage,
              );
            }
            notifyListeners();
          },
          errorFilter: ApiErrorFilter(
            (error) => error.code == 412 || error.code == 403,
            ErrorReaction.localHandler,
          ),
          debugName: cmdAddMessage,
        )
        ..errors.listen((ex, _) {
          final apiError = ex!.error as ApiException;

          /// TODO adjust to chatmessagesource
          items.removeLast();
          lastMessage = null;
          refreshItemCOunt();
          if (apiError.message?.contains('limited to 15 people') ?? false) {
            di<InteractionManager>().pushToastWithBuilder(
              (context) => ToastConfig(context.l10n.messagingLimitPerDay),
            );
          } else if (apiError.message?.contains('Permission denied') ?? false) {
            di<InteractionManager>().pushToastWithBuilder(
              (context) => ToastConfig(
                context.l10n.defaultMessagingRestriction,
              ),
            );
          }
        });

Those look pretty good to me. Is that what you had in mind, @escamoteur?

escamoteur commented 3 weeks ago

that looks great!!!