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

Move class "extends"/implements/with to a new line when generics take a lot of space #1348

Closed
rrousselGit opened this issue Dec 31, 2023 · 2 comments

Comments

@rrousselGit
Copy link

Hello!

Consider the following code I dealt with earlier:

abstract class StreamNotifierProviderBase<
        NotifierT extends AsyncNotifierBase<T>,
        T> extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
    with FutureModifier<T> {
// ...
}

With the way extends and generics are mixed up, this is fairly unreadable.
It's very hard with a quick glance to know what this class extends exactly.

I propose formatting such cases like the with:

abstract class StreamNotifierProviderBase<
        NotifierT extends AsyncNotifierBase<T>,
        T>
    extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
    with FutureModifier<T> {
// ...
}
@munificent
Copy link
Member

Yeah, I agree the current formatting is pretty bad. The new in-progress style gives you:

abstract class StreamNotifierProviderBase<
  NotifierT extends AsyncNotifierBase<T>,
  T
> extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
    with FutureModifier<T> {
  // ...
}

Which isn't much better, I think. The rule should probably be that if the class type parameters split, then we force any subsequent clauses to move to their own lines too, like:

abstract class StreamNotifierProviderBase<
      NotifierT extends AsyncNotifierBase<T>,
      T
    >
    extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
    with FutureModifier<T> {
  // ...
}

I don't plan to tweak the formatting in the old style, but I'll leave this issue open to improve how the new style handles this case.

@munificent
Copy link
Member

I recently (#1511) fixed this in the forthcoming tall style formatter. With that change, you get:

abstract class StreamNotifierProviderBase<
  NotifierT extends AsyncNotifierBase<T>,
  T
>
    extends ProviderWithNotifier<AsyncValue<T>, NotifierT>
    with FutureModifier<T> {
  // ...
}

I debated whether the type parameters should be indented so that the > lines up with the other clauses, but ultimately decided not to indent. I'm still on the fence. Fortunately, it rarely comes up in practice.

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

No branches or pull requests

2 participants