Skip to content

Add parser support for --lines, --offset, --length#627

Open
ZacSweers wants to merge 10 commits into
facebook:mainfrom
ZacSweers:z/lines
Open

Add parser support for --lines, --offset, --length#627
ZacSweers wants to merge 10 commits into
facebook:mainfrom
ZacSweers:z/lines

Conversation

@ZacSweers

Copy link
Copy Markdown
Contributor

Resolves #218 and includes IDE support as well.

Note that I opted to match GJF's current behavior and allow statement-level boundaries (the issue described function-level granularity only, but that's not what I see GJF do).

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 13, 2026

@hick209 hick209 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just haven't finished core/src/main/java/com/facebook/ktfmt/format/Formatter.kt, but here's the start.

I'll resume tomorrow

return parts[1].takeIf { parts[0] == key || parts.size == 2 }
}

private fun String?.toIntOrNull(): Int? {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason why we re-implement this here?
Isn't there an equivalent in the core Kotlin API?

Comment on lines +167 to +184
arg == "--lines" || arg == "--line" -> {
val value =
nextValue() ?: return ParseResult.Error("required value was not provided for: $arg")
when (val result = parseLineRanges(lineRanges, value)) {
LineRangeParseResult.Success -> Unit
is LineRangeParseResult.Error -> return ParseResult.Error(result.message)
}
}
arg.startsWith("--lines=") ->
when (val result = parseLineRanges(lineRanges, parseKeyValueArg("--lines", arg))) {
LineRangeParseResult.Success -> Unit
is LineRangeParseResult.Error -> return ParseResult.Error(result.message)
}
arg.startsWith("--line=") ->
when (val result = parseLineRanges(lineRanges, parseKeyValueArg("--line", arg))) {
LineRangeParseResult.Success -> Unit
is LineRangeParseResult.Error -> return ParseResult.Error(result.message)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could likely be simplified to something like this or similar. Same goes for the options below

    arg.startsWith("--line") -> {
      val argSplit = arg.split('=', limit = 2)
      val key = argSplit.first()
      if (key != "--lines" && key != "--line") {
        ParseResult.Error("Unexpected option: $key")
      }
      val value = if (argSplit.size > 1) argSplit.last() else nextValue() ?: return ParseResult.Error("required value was not provided for: $key")

      when (val result = parseLineRanges(lineRanges, value)) {
        LineRangeParseResult.Success -> Unit
        is LineRangeParseResult.Error -> return ParseResult.Error(result.message)
      }
    }

Comment on lines +269 to +270
private fun invalidInt(flag: String, value: String?): String =
"invalid integer value for $flag: $value"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't feel like this method adds much value. I won't stop if you like it though

@Test
fun `parseOptions recognizes --lines ranges`() {
val parsed =
assertSucceeds(ParsedArgs.parseOptions(arrayOf("--lines=1:3,5", "--lines", "7", "foo.kt")))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe create a helper to reduce boilerplate here (not on you really, but since you are touching it, you know 😅)

// In this test file here
private fun parseOptions(vararg options: String) =
    ParsedArgs.parseOptions(options.toTypedArray())

Comment on lines +383 to +387
private fun lineRanges(vararg ranges: Range<Int>): RangeSet<Int> {
return ranges(*ranges)
}

private fun ranges(vararg ranges: Range<Int>): RangeSet<Int> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why both of those here? Let's just pick one and go with that

Comment on lines +378 to +379
parsedArgs.lineRanges.addAll(lineRanges)
parsedArgs.characterRanges.addAll(characterRanges)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The logic here doesn't feel like it should live here for me.
Thoughts?

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support formatting of specific line ranges

2 participants