Skip to content
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

Minor fixes to formatting to prepare for KtLint 1.0 #1183

Closed
wants to merge 15 commits into from

Conversation

TWiStErRob
Copy link
Contributor

"Preparation" as in reduce the future PR size by applying manual intervention requiring fixes earlier and fix things that prevent spotlessApply from executing.

The changes are exhaustive, but intentionally didn't include the fix for (i.e. #1076 will fail with this error still):

> Task :paparazzi:spotlessKotlin FAILED
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\ImageUtils.kt':
Error on line: 72, column: 9
rule: standard:property-naming
Property name should start with a lowercase letter and use camel case

because that will be fixed in ktlint 1.0.2 or 1.1.0: pinterest/ktlint#2352

Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\sample\ResourcesDemo.kt':
Error on line: 31, column: 5
rule: standard:function-naming
Function name should start with a lowercase letter (except factory methods) and use camel case
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\sample\ResourcesDemo.kt':
Error on line: 27, column: 11
rule: standard:property-naming
Property name should use the screaming snake case notation when the value can not be changed
Step 'ktlint' found problem in 'src\test\java\app\cash\paparazzi\sample\ComposeA11yTest.kt':
Error on line: 56, column: 39
rule: standard:discouraged-comment-location
A comment in a 'value_argument_list' is only allowed when placed on a separate line
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\interceptors\MatrixMatrixMultiplicationInterceptor.kt':
Error on line: 19, column: 11
rule: standard:property-naming
Property name should start with a lowercase letter and use camel case
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\resources\Pseudolocalizer.kt':
Error on line: 351, column: 15
rule: standard:property-naming
Property name should start with a lowercase letter and use camel case
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\resources\RepositoryLoader.kt':
Error on line: 124, column: 15
rule: standard:property-naming
Property name should start with a lowercase letter and use camel case
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\interceptors\MatrixVectorMultiplicationInterceptor.kt':
Error on line: 23, column: 2
rule: standard:max-line-length
Exceeded max line length (140)
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\resources\Pseudolocalizer.kt':
Error on line: 22, column: 1
rule: standard:no-consecutive-comments
an EOL comment may not be preceded by a KDoc. Reversed order is allowed though when separated by a newline.
Step 'ktlint' found problem in 'src\test\java\app\cash\paparazzi\internal\resources\PseudolocaleGeneratorTest.kt':
Error on line: 85, column: 1
rule: standard:max-line-length
Exceeded max line length (140)
Step 'ktlint' found problem in 'src\main\java\app\cash\paparazzi\internal\interceptors\MatrixMatrixMultiplicationInterceptor.kt':
Error on line: 38, column: 15
rule: standard:function-naming
Function name should start with a lowercase letter (except factory methods) and use camel case
Step 'ktlint' found problem in 'src\test\projects\lifecycle-usages\src\test\java\app\cash\paparazzi\plugin\test\LifecycleUsageTest.kt':
Error on line: 21, column: 13
rule: standard:function-naming
Function name should start with a lowercase letter (except factory methods) and use camel case
handleParsingError(file, e)
} catch (e: XmlPullParserException) {
handleParsingError(file, e)
} catch (e: XmlSyntaxException) {
handleParsingError(file, e)
} catch (e: RuntimeException) {
// KXmlParser throws RuntimeException for an undefined prefix and an illegal attribute name.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

obsoleted by suggestion to add:

ktlint_standard_discouraged-comment-location = disabled

@@ -347,9 +347,11 @@ class PseudoMethodAccent : PseudoMethodImpl() {
return currentIndex
}

// Yes, "fiveteen".
private val EXPANSION_STRING = "one two three four five six seven eight nine ten eleven " +
"twelve thirteen fourteen fiveteen sixteen seventeen nineteen twenty"
Copy link
Collaborator

Choose a reason for hiding this comment

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

move to line 35

@@ -43,7 +43,8 @@ internal class ModuleResourceRepository private constructor(
resourceDirectories: List<File>
): LocalResourceRepository {
return ModuleResourceRepository(
displayName = "", // TODO
Copy link
Collaborator

Choose a reason for hiding this comment

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

An anti-bikeshedding Kotlin linter with built-in formatter

yea....

let's disable this rule in .editorconfig

ktlint_standard_discouraged-comment-location = disabled

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, I tried doing this locally and came across the error mentioned here:
pinterest/ktlint#2338 (comment)

That looks to be fixed here: pinterest/ktlint#2371
but set to be released in 1.1, so I think this PR will likely hang out (or maybe we can merge some of the changes without bumping to 1.0?) since the "discouraged comment location" is more opinionated than I like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR does NOT bump the ktlint version, that's the point. But I'll have to retry some of this it seems based on your feedback with the intellij style re-applied.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, sounds good. i can also continue work on this effort, if that's easier for you.

resultVec[resultVecOffset + 0] =
lhsMat[I(0, 0, lhsMatOffset)] * x + lhsMat[I(1, 0, lhsMatOffset)] * y + lhsMat[I(2, 0, lhsMatOffset)] * z + lhsMat[I(3, 0, lhsMatOffset)] * w
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert and add "ktlint:standard:property-naming" to @Suppress above intercept method

// #define I(_i, _j) ((_j)+ 4*(_i))
return offset + j + 4 * i
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

revert all this, since you've added "ktlint:standard:property-naming" to @Suppress above intercept method?

@@ -84,7 +84,8 @@ internal class Renderer(
val resourceDirPath = Paths.get(dir)
AarSourceResourceRepository.create(
resourceDirectoryOrFile = resourceDirPath,
libraryName = resourceDirPath.parent.fileName.name // segment before /res
// segment before /res
libraryName = resourceDirPath.parent.fileName.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

obsoleted by suggestion to add:

ktlint_standard_discouraged-comment-location = disabled

assertThat(getElement(0))
.isEqualTo("${bidiWordStart}First$bidiWordEnd ${bidiWordStart}Test$bidiWordEnd ${bidiWordStart}String$bidiWordEnd")
assertThat(getElement(1))
.isEqualTo("${bidiWordStart}Second$bidiWordEnd ${bidiWordStart}Test$bidiWordEnd ${bidiWordStart}String$bidiWordEnd")
Copy link
Collaborator

Choose a reason for hiding this comment

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

what changes led to these? revert?

Default(null),
Arabic("ar"),
Accent("en-rXA"),
Bidi("ar-rXB")
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

attr = BasicAttrResourceItem(attr.name, attr.sourceFile, attr.visibility, attr.description, attr.groupName, DEFAULT_ATTR_FORMATS, emptyMap(), emptyMap())
attr = BasicAttrResourceItem(
attr.name, attr.sourceFile, attr.visibility, attr.description, attr.groupName, DEFAULT_ATTR_FORMATS, emptyMap(), emptyMap()
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

unneeded?

handleParsingError(file, e)
} catch (e: XmlPullParserException) {
handleParsingError(file, e)
} catch (e: java.lang.RuntimeException) {
// KXmlParser throws RuntimeException for an undefined prefix and an illegal attribute name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

obsoleted by suggestion to add:

ktlint_standard_discouraged-comment-location = disabled

handleParsingError(file, e)
} catch (e: XmlPullParserException) {
handleParsingError(file, e)
} catch (e: XmlSyntaxException) {
handleParsingError(file, e)
} catch (e: RuntimeException) {
// KXmlParser throws RuntimeException for an undefined prefix and an illegal attribute name.
Copy link
Collaborator

Choose a reason for hiding this comment

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

obsoleted by suggestion to add:

ktlint_standard_discouraged-comment-location = disabled

@jrodbx
Copy link
Collaborator

jrodbx commented Jan 9, 2024

Applied suggested feedback, manually squashed and pushed. Thanks!

@jrodbx jrodbx closed this Jan 9, 2024
@TWiStErRob TWiStErRob deleted the ktlint-prep branch January 9, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants