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

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

Closed
escamoteur opened this issue May 6, 2024 · 8 comments · Fixed by #1514
Closed

Comments

@escamoteur
Copy link

escamoteur commented May 6, 2024

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
Copy link
Author

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

@srawlins
Copy link
Member

srawlins commented May 6, 2024

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
Copy link
Author

escamoteur commented May 6, 2024 via email

@munificent
Copy link
Member

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
Copy link
Member

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
Copy link

@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
Copy link
Member

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
Copy link
Author

that looks great!!!

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

Successfully merging a pull request may close this issue.

4 participants