Add parser support for --lines, --offset, --length#627
Conversation
hick209
left a comment
There was a problem hiding this comment.
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? { |
There was a problem hiding this comment.
Any reason why we re-implement this here?
Isn't there an equivalent in the core Kotlin API?
| 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) | ||
| } |
There was a problem hiding this comment.
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)
}
}
| private fun invalidInt(flag: String, value: String?): String = | ||
| "invalid integer value for $flag: $value" |
There was a problem hiding this comment.
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"))) |
There was a problem hiding this comment.
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())
| private fun lineRanges(vararg ranges: Range<Int>): RangeSet<Int> { | ||
| return ranges(*ranges) | ||
| } | ||
|
|
||
| private fun ranges(vararg ranges: Range<Int>): RangeSet<Int> { |
There was a problem hiding this comment.
Why both of those here? Let's just pick one and go with that
| parsedArgs.lineRanges.addAll(lineRanges) | ||
| parsedArgs.characterRanges.addAll(characterRanges) |
There was a problem hiding this comment.
The logic here doesn't feel like it should live here for me.
Thoughts?
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).