Skip to content

Conversation

@flwyd
Copy link
Contributor

@flwyd flwyd commented Oct 11, 2021

Fixes #176.

The ktfmt tool doesn't currently support the --lines flag from google-java-format, so we use AttemptFakeRangeFormatting. Based on cursory testing this mostly works if applied to a logical code block, but it will align it will indent relative to the first column, as if this were the only text in the file, rather than keeping the existing indent.

flwyd added 3 commits October 8, 2021 18:46
Unfortunately, ktfmt doesn't yet support a --lines option to format a
range. See facebook/ktfmt#218 for the
feature request.
@google-cla google-cla bot added the cla: yes label Oct 11, 2021
Copy link
Contributor

@dbarnett dbarnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for chasing down logistics and doing this!

@dbarnett
Copy link
Contributor

Based on cursory testing this mostly works if applied to a logical code block, but it will align it will indent relative to the first column, as if this were the only text in the file, rather than keeping the existing indent.

To clarify, this means it'll tend to dedent to col 1 and you'll have to select and manually indent after formatting a block, right? Might note that caveat in the help somewhere.

endif
if executable(l:exec[0])
return 1
elseif !empty(l:exec[0]) && l:exec[0] isnot# 'ktfmt'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is intended to allow non-default formatters to work, but in this case exec[0] would never be ktfmt, since that's not the default.

This particular pattern is only present for google-java-format, and I think there only because we aren't doing the splitting that you have here on L31, so we just give up trying to check for an executable.

I think we probably want one or the other: either split to find an executable ("java", here), and check whether it's runnable, or alternatively skip that check if the flag value's been overridden. Since there's no single ktfmt binary to run, I think probably the former?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did something fancy, see my response to @dbarnett above.

Copy link
Member

@malcolmr malcolmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support ktfmt to format Kotlin

3 participants