-
Notifications
You must be signed in to change notification settings - Fork 289
Enforce ktlint max-line-length with Spotless gradle/init.gradle.kts #609
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
Conversation
No region tags are edited in this PR.This comment is generated by snippet-bot.
|
48691b6 to
3f1660d
Compare
|
I am supportive of this, but I don't see how the modification of the lint rules follow the changes that are suggested in the PR - are there some changes that are manual instead of generated by the linter? Examples on the commits. |
| .wrapContentWidth() | ||
| .padding(8.dp), | ||
| style = MaterialTheme.typography.titleSmall | ||
| style = MaterialTheme.typography.titleSmall, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added trailing comma
| } | ||
| composable( | ||
| "details/{id}", | ||
| route = "details/{id}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Named parameter added
| ) { | ||
| Icon( | ||
| painter = if (isPressed) painterResource(id = selectedImage) else painterResource(id = unselectedImage), | ||
| painter = if (isPressed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brace style fixed
| shape = SurfaceEntity.Shape.Quad(FloatSize2d(1.0f, 1.0f)), | ||
| surfaceProtection = SurfaceEntity.SurfaceProtection.SURFACE_PROTECTION_PROTECTED | ||
| surfaceProtection = | ||
| SurfaceEntity.SurfaceProtection.SURFACE_PROTECTION_PROTECTED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? I would expect an indent here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also expect an indent here. I'm not sure why Spotless or Android Studio suggested this formatting. If I have time, I can check to see if Spotless will let me add the indentation. I would normally expect Spotless to fix this, so maybe some of the other lint rules can be improved.
|
Whenever possible, the changes come from In general, I think it's better to try and simplify the Spotless configuration so that it uses more and more of the default Android Studio style guides. Currently there are a lot of disabled rules, and I'm not sure how long those disabled rules interact with each other. I'm also not sure why there are two different line length rules that were disabled. Overall, I agree with your confusion. If we can fix more of these style issues with Spotless configuration, then we can spend less time discussing the indentation, named parameters, and trailing commas. The primary purpose of this change is to provide a step towards a more strict and standard Spotless configuration. If it's easier, you are welcome to close this pull request and create a new one with additional changes that go beyond this step. You are also welcome to merge this version and then create additional changes that build on it. |
|
Discussed with Cartland about how we can move forward on this. I think we need to enable more of the Spotless rules one-by-one, and then hopefully enabling this rule will only change the line lengths and not change other parts of the code. So we don't need to enforce line length later down the pipeline. Thanks Cartland! We'll take a closer look at how to get to this state. |
Snippets from this repository are included in documentation at developer.android.com. Long lines are difficult to read in these snippets (and in general). We previously disabled
ktlintformax-line-lengthbecause it was difficult to enforce across all snippets. Now we're seeing the challenge appear in the documentation publishing process. Since the snippet line length is not enforced in the code base, teams are adding manual warnings and checks at publishing time to make sure that the line length is not too long.I am creating this pull request to show which changes will allow
max-line-lengthto be enforced. I will ask teams to review these changes to make sure they don't cause other side effects in the documentation or build process. After everyone is satisfied, we can enforce the line lengths.