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

If case when should prefer adding a new line to when and not to the inner variables [New style] #1596

Open
FMorschel opened this issue Nov 7, 2024 · 1 comment

Comments

@FMorschel
Copy link

If you have a code like:

if (node.parent case MethodInvocation(:var target) when target is SimpleIdentifier?)

Today it formats like this:

if (node.parent case MethodInvocation(
  :var target,
) when target is SimpleIdentifier?)

In this case I think it should prefer to add a new line before when rather than making a whole new line for :var target.

@lucavenir
Copy link

I'd like to add my proposal about this. Pattern matching formatting right now is kind-of ugly, and hard to read if you ask me. So imho this issue is bigger than this single case.

I'm not sure if this is recognized as an area of improvement and/or if there's an umbrella issue for this, but there could be many formatting possibilities when it comes to pattern matching.
Also a lot depends on proposals that might shorten the current's pattern matching syntax.

I just want to underline that the following proposal is naive and probably might need to improve.

I'd propose to format the above as:

if (
  node.parent case MethodInvocation(:var target)
  when target == whatever
)

Which consumes more white space, but at least the pattern match is crystal clear. IMHO you can easily read where the if statement begins, where it ends, which variables are matched and against who. The when clause has the same magnitude as the case clause.

AFAIK a little more white space should harmless, so... let me show another example.

A more complex case could be the following (formatted "as of now"):

if (err
    case DioException(
          type: DioExceptionType.badCertificate || DioExceptionType.cancel
        ) ||
        DioException(
          type: DioExceptionType.badResponse,
          response: Response(statusCode: != null && >= 200 && <= 500)
        ) when Random().nextInt(100) > 10) {
  return super.onError(err, handler);
}

Instead, we could format the above as:

if (
  err case
    DioException(
      type: DioExceptionType.badCertificate || DioExceptionType.cancel,
    )
    ||
    DioException(
      type: DioExceptionType.badResponse,
      response: Response(statusCode: != null && >= 200 && <= 500),
    )
  when
    Random().nextInt(100) > 10
  ) {
  return super.onError(err, handler);
}

Again, I'm not sure people would like this. My goal with this comment is to ask you guys if this has been ever thought over / what's the current state of proposal about this problem, and if this is even recognized as a problem. It might be a starting point.

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