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

Regression in 2.6.0 #683

Open
bsideup opened this issue Sep 13, 2024 · 3 comments
Open

Regression in 2.6.0 #683

bsideup opened this issue Sep 13, 2024 · 3 comments

Comments

@bsideup
Copy link

bsideup commented Sep 13, 2024

Prettier-Java 0.6.0

# Options (if any):
--print-width 120

It looks like #632 , despite being a great improvement overall, introduced a regression when the first method call needs to be multi-line due to long arguments, leading to a closing ) misaligned.

Input:

public static final MethodDescriptor<ByteBuf, ByteBuf> NEW_TUNNEL_METHOD = MethodDescriptor
		.newBuilder(ByteBufMarshaller.INSTANCE, ByteBufMarshaller.INSTANCE)
		.setFullMethodName(TUNNEL_SERVICE + "/new")
		.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
		.build();

Output:

public static final MethodDescriptor<ByteBuf, ByteBuf> NEW_TUNNEL_METHOD = MethodDescriptor.newBuilder(
		ByteBufMarshaller.INSTANCE,
		ByteBufMarshaller.INSTANCE
	)
		.setFullMethodName(TUNNEL_SERVICE + "/new")
		.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
		.build();

Expected behavior:

public static final MethodDescriptor<ByteBuf, ByteBuf> NEW_TUNNEL_METHOD = MethodDescriptor
		.newBuilder(ByteBufMarshaller.INSTANCE, ByteBufMarshaller.INSTANCE)
		.setFullMethodName(TUNNEL_SERVICE + "/new")
		.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
		.build();

Alternative expected behavior:

public static final MethodDescriptor<ByteBuf, ByteBuf> NEW_TUNNEL_METHOD = MethodDescriptor.newBuilder(
			ByteBufMarshaller.INSTANCE,
			ByteBufMarshaller.INSTANCE
		)
		.setFullMethodName(TUNNEL_SERVICE + "/new")
		.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
		.build();

See bsideup/grpc-bidi@bd35134

@bsideup bsideup changed the title Regression it 2.6.0 Regression in 2.6.0 Sep 13, 2024
@jtkiesel
Copy link
Contributor

Thank you for creating this issue! However, I believe this is intended behavior. At least, it mimics Prettier's own formatting for a similar code snippet in TypeScript, which is what most (if not all) of the formatting decisions in #632 were based upon.

Prettier Java example:

class Example {

	public static final MethodDescriptor<ByteBuf, ByteBuf> NEW_TUNNEL_METHOD = MethodDescriptor.newBuilder(
		ByteBufMarshaller.INSTANCE,
		ByteBufMarshaller.INSTANCE
	)
		.setFullMethodName(TUNNEL_SERVICE + "/new")
		.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
		.build();
}

Prettier TypeScript example:

class Example {
	public static readonly NEW_TUNNEL_METHOD: MethodDescriptor<ByteBuf, ByteBuf> = MethodDescriptor.newBuilder(
		ByteBufMarshaller.INSTANCE,
		ByteBufMarshaller.INSTANCE,
	)
		.setFullMethodName(TUNNEL_SERVICE + "/new")
		.setType(MethodDescriptor.MethodType.BIDI_STREAMING)
		.build();
}

Because of that, I'm doubtful that we will revert this change, as we're trying our best to keep Prettier Java aligned with Prettier's own formatting decisions, specifically in TypeScript (which tends to be the "native" language most similar to Java).

@lppedd
Copy link

lppedd commented Sep 17, 2024

I'll be honest tho, the current actual behavior looks noticeable worse than what we'd expect.
It's like seeing a lost parenthesis in the middle of the code.

@bsideup
Copy link
Author

bsideup commented Sep 17, 2024

@jtkiesel ah, thanks, I missed that! I wonder if it would be considered bad for TypeScript too and is just yet to be fixed. Otherwise, I agree with @lppedd that it looks like a black sheep of otherwise beautifully formatted code.

Would you be open to reconsider the current behavior (even if it means slightly diverging from TS, at least until they reconsider it too)? I do agree and accept all the opinionatedness of prettier-java except this one that does look like a bug in the formatter and does not match any other formatting logic in the rest of code 😅

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

3 participants