-
Notifications
You must be signed in to change notification settings - Fork 74
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
Automatically manage trailing commas when running with --google-style #427
Conversation
@davidtorosyan @hick209 @cgrushko I know we talked about this idea a bit several months ago. I've limited the impl to --google-style, but it could easily be expanded to more styles, or possibly a separate flag. |
5aee7c8
to
663b6b9
Compare
@hick209 Is anyone available to review? |
Hey @nreid260, sorry I'm out on paternity leave. I'll ping the group internally to get into this though |
@strulovich has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks ok, and terribly sorry you had to wait for so long. A bunch of mostly style issues and requirement for extra documentation so it's easier to jump to and we should be good to go.
.let { addRedundantElements(it, options) } | ||
.let { convertLineSeparators(it, Newlines.guessLineSeparator(kotlinCode)!!) } | ||
|
||
return if (shebang.isNotEmpty()) { shebang + "\n" + formatted } else { formatted } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: If we're restyling this, maybe add this check to the let chain as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/** Remove and insert trialing commas based on how the code would format without them. */ | ||
val manageTrailingCommas: Boolean = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate more? The best way would be with 1-2 code examples that explain what the user can expect to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
file.accept( | ||
object : KtTreeVisitorVoid() { | ||
override fun visitKtElement(element: KtElement) { | ||
trailingCommaSuggestor.takeElement(element) | ||
super.visitElement(element) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just use forEachDescendantOfType
or collectDescendantsOfType
for shorter and simpler code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
takeElement decides which elements to track. It would be error prone to duplicate that knowledge into the traversal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understood your comment, I was suggesting:
file.forEachDescendantOfType<KtElement> {
trailingommaSuggestor.takeElement(element)
}
|
||
fun takeElement(element: KtElement) { | ||
if (!element.text.contains("\n")) { | ||
return // Only suggest trailing commas where there is already a line break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, a bunch of code examples above the method would really help people maintaining this years from now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
} | ||
|
||
private fun PsiElement.leftLeafIgnoringCommentsAndWhitespace(): PsiElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add examples code and what it means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// TODO(nickreid): What about: | ||
// - function-type param lists | ||
// - destructuring declarations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not Meta style, so let's try to stick to something that's more applicable: Can you open issues and link to them from the TODO instead? You can have the issues assigned to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
663b6b9
to
91123b8
Compare
result.replace(element.startOffset, element.endOffset, replacement) | ||
} | ||
|
||
return result.toString() | ||
} | ||
|
||
fun addRedundantElements(code: String, options: FormattingOptions): String { | ||
// TODO(nickreid): Generalize this for addtional sugested elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here re: TODOs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
return kotlinCode | ||
.let { convertLineSeparators(it) } | ||
.let { sortedAndDistinctImports(it) } | ||
.let { dropRedundantElements(it, options) } | ||
.let { prettyPrint(it, options, "\n") } | ||
.let { addRedundantElements(it, options) } | ||
.let { convertLineSeparators(it, Newlines.guessLineSeparator(kotlinCode)!!) } | ||
.let { if (shebang.isEmpty()) it else shebang + "\n" + it } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love it
Commas are added/removed according to the following rules: - Lambda param lists => remove - Single element lists => remove - Lists that fit on one line => remove - All other lists (multiline lists) => add
91123b8
to
b3e4fa6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
file.accept( | ||
object : KtTreeVisitorVoid() { | ||
override fun visitKtElement(element: KtElement) { | ||
trailingCommaSuggestor.takeElement(element) | ||
super.visitElement(element) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understood your comment, I was suggesting:
file.forEachDescendantOfType<KtElement> {
trailingommaSuggestor.takeElement(element)
}
@strulovich has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Ooooh, yeah that seems fine. |
@strulovich merged this pull request in fa78077. |
* upstream/main: Bump version to 0.47-SNAPSHOT Bump version to 0.47 Reformat Kotlin files Automatically manage trailing commas when running with --google-style (facebook#427)
* upstream/main: Bump version to 0.47-SNAPSHOT Bump version to 0.47 Reformat Kotlin files Automatically manage trailing commas when running with --google-style (facebook#427)
Commas are added/removed according to the following rules: