Fix a bunch of build warnings #283
Conversation
facebook-github-bot
added
the
CLA Signed
|
Looks like this depends on Guava 31 |
nreid260
force-pushed
the
fix_warnings
branch
2 times, most recently
from
b770107
to
48de712
Compare
|
Java 13 still has failures - can you see the logs? (not sure about the permissions here) |
|
I can see them, but I don't understand what would be different. Do you have a minute to take a look? The goal here is just to stop kotlinc from spamming so much. |
|
No idea; does it build successfully locally for you on Java 13? |
nreid260
force-pushed
the
fix_warnings
branch
6 times, most recently
from
c591025
to
e88be9e
Compare
nreid260
force-pushed
the
fix_warnings
branch
from
e88be9e
to
8af27d2
Compare
|
Looks like it was some kind of incompatibility with Guava 31 |
|
Poke |
| when { | ||
| length < 0 -> return EMPTY_RANGE | ||
| length == 0 -> 1 // 0 stands for "format the line under the cursor" | ||
| else -> length |
There was a problem hiding this comment.
should this be else -> 1, at least to preserve the current behavior?
There was a problem hiding this comment.
This should preserve the current behaviour. length = 1 isn't a case, it's the behaviour when length == 0.
| val builder = ImmutableMap.builder<Int, Int>() | ||
| val builder = LinkedHashMap<Int, Int>() | ||
| for (tok in toks) { | ||
| builder.put(tok.position, tok.column) | ||
| } | ||
| return builder.build() | ||
| return ImmutableMap.copyOf(builder) |
There was a problem hiding this comment.
what's the build warning here?
There was a problem hiding this comment.
ImmutableMap.Builder.build() throws if there are are duplicate keys. This caused enough confusion that a preferred, explicit method buildOrThrow was added in Guava 31.0. Google builds from the newest version of Guava. https://guava.dev/releases/31.0.1-jre/api/docs/com/google/common/collect/ImmutableMap.Builder.html#build()
There was a problem hiding this comment.
Got it. I think it's fine this time, but we'll keep running into these problems, since Meta / OSS isn't aware of Guava 31.
Don't know if policy allows that, but you may want to consider adding Guava 29 to third-party (and limiting its visibility to ktfmt).
| @@ -148,7 +148,7 @@ | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.jetbrains.kotlin</groupId> | |||
| <artifactId>kotlin-stdlib</artifactId> | |||
| <artifactId>kotlin-stdlib-jdk7</artifactId> | |||
There was a problem hiding this comment.
what's the build warning here?
There was a problem hiding this comment.
I believe this is needed for the changes to core/src/test/java/com/facebook/ktfmt/cli/MainTest.kt
|
@cgrushko has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| for (element in name) { | ||
| if (!Character.isAlphabetic(element.toInt())) { | ||
| for (char in name) { | ||
| if (!Character.isAlphabetic(char.code)) { | ||
| continue | ||
| } | ||
| if (first) { | ||
| firstUppercase = Character.isUpperCase(element) | ||
| firstUppercase = Character.isUpperCase(char) | ||
| first = false | ||
| } | ||
| hasUppercase = hasUppercase or Character.isUpperCase(element) | ||
| hasLowercase = hasLowercase or Character.isLowerCase(element) | ||
| hasUppercase = hasUppercase or Character.isUpperCase(char) | ||
| hasLowercase = hasLowercase or Character.isLowerCase(char) |
There was a problem hiding this comment.
If you're changing this how about using the Kotlin extension methods?
| private val root = createTempDir() | ||
| private val root = createTempDirectory().toFile() |
There was a problem hiding this comment.
createTempDir is marked deprecated
| @@ -2069,7 +2069,7 @@ class FormatterTest { | |||
| deduceMaxWidth = true) | |||
|
|
|||
| @Test | |||
| fun `handle ? for nullalble types`() = | |||
| fun `handle qmark for nullalble types`() = | |||
There was a problem hiding this comment.
"question mark" please
Let's avoid abbreviations for easier readability
No description provided.