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

Type names in a line comment may break the line #1076

Closed
netdpb opened this issue May 3, 2021 · 4 comments
Closed

Type names in a line comment may break the line #1076

netdpb opened this issue May 3, 2021 · 4 comments
Labels

Comments

@netdpb
Copy link

netdpb commented May 3, 2021

addComment() replaces all the spaces in the format string with · for nonbreaking space, but not spaces in type arguments.

builder.addComment("this is a long line with a possibly long type: %T", someLongParamterizedTypeName);

The above could end up generating code like

// this is a long line with a possibly long type: List<Map<String, Collection<Map<WackyKey,
  OhNoThisDoesNotCompile>>>>
@Egorand Egorand added the bug label May 3, 2021
@ZacSweers
Copy link
Collaborator

Thanks for the report and repro case. Unfortunately the fix doesn't look too simple, so I would suggest working around this by writing the type to a string first and then adding that as a literal to the comment

val longName = someLongParamterizedTypeName.toString()
builder.addComment("this is a long line with a possibly long type: $longName");

not as pretty, but should workaround this for now

ZacSweers added a commit that referenced this issue Sep 20, 2021
@glureau
Copy link

glureau commented Oct 10, 2021

What do we expect here?

// this is a long line with a possibly long type: List<Map<String, Collection<Map<WackyKey, OhNoThisDoesNotCompile>>>>

or

// this is a long line with a possibly long type: List<Map<String, Collection<Map<WackyKey,
//   OhNoThisDoesNotCompile>>>>

First option is a 1~2 char changes I suppose + unit tests, if we choose to never wrap spaces in a long type, I can create the PR if it can help.

Second option looks like #744 in the way line wrapping today doesn't have the context of "in comment block" or "in string" while wrapping. Should we add this concept there (in LineWrapper)? Because it could mean that the comment/big string blocks are known before wrapping, but lost while emitting string, and re-captured from string to process the line wrapping, looks a bit tricky.

EDIT: just discovered #532 and #274 maybe the same discussion?

@netdpb
Copy link
Author

netdpb commented Oct 11, 2021

I personally think the first option is fine. I think that's what the code does with the non-type string.

@Egorand
Copy link
Collaborator

Egorand commented Oct 15, 2021

Yep, agree that option 1 is fine. Type declarations aren't usually wrapped in handwritten code, and I think it's fine to ignore the line limit for this case.

glureau pushed a commit to glureau/kotlinpoet that referenced this issue Oct 15, 2021
glureau pushed a commit to glureau/kotlinpoet that referenced this issue Oct 15, 2021
glureau pushed a commit to glureau/kotlinpoet that referenced this issue Oct 15, 2021
Egorand added a commit that referenced this issue Oct 20, 2021
Fix #1076: Type names in a line comment may break the line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants