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

Update KTLint #585

Merged
merged 17 commits into from
Nov 19, 2024
Merged

Update KTLint #585

merged 17 commits into from
Nov 19, 2024

Conversation

stevenzeck
Copy link
Contributor

@stevenzeck stevenzeck commented Nov 5, 2024

This PR updates the ktlint gradle plugin to the latest version. In doing so, we are also updating ktlint itself to 1.0.0, and there seems to be a lot of changes there.

Note that ktlint_code_style is set to intellij_idea. That was the default style before 1.0 so it should match our current. There is an option for android_studio too.

@stevenzeck stevenzeck marked this pull request as ready for review November 8, 2024 00:56
@stevenzeck
Copy link
Contributor Author

stevenzeck commented Nov 8, 2024

@mickael-menu @qnga This is ready for review. Simply comment out the rule in .editorconfig and run ktlintCheck to see where the issues still are and decide whether they're something you want fixed or not. We can also run ktlintFormat to let it autofix what it can for some of the rules. Note that the two trailing comma rules would fix 500 some files.

Some of the rules seemed to either get more strict or were fixed, like no semicolons now applying to enum classes.

@qnga qnga self-requested a review November 8, 2024 07:58
Copy link
Member

@qnga qnga left a comment

Choose a reason for hiding this comment

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

Looks good to me. We might be able to re-enable some rules after removing some deprecated code.

@stevenzeck stevenzeck marked this pull request as draft November 9, 2024 01:09
@stevenzeck
Copy link
Contributor Author

This is ready for review again. The auto-format was run and committed for individual rules, so those can be reverted easily if needed. The only one I'm not sure about that I did run was wrapping. If you don't like those changes, I'll revert that commit. Otherwise, I think everything else looks good.

Note that I left trailing-comma-on-call-site and trailing-comma-on-declaration-site in there as it changes 357 files and 162 files respectively.

@stevenzeck stevenzeck marked this pull request as ready for review November 15, 2024 23:39
@qnga
Copy link
Member

qnga commented Nov 16, 2024

Indeed, I would go without the wrapping rule. Constructors look cumbersome with it. @mickael-menu?

@stevenzeck
Copy link
Contributor Author

Looking at it again, maybe it's not horrible. Generic type and type parameter each has its own line.

Old

public interface TtsEngine<S : TtsEngine.Settings, P : TtsEngine.Preferences<P>,
    E : TtsEngine.Error, V : TtsEngine.Voice> : Configurable<S, P>, Closeable {

New

public interface TtsEngine<
    S : TtsEngine.Settings,
    P : TtsEngine.Preferences<P>,
    E : TtsEngine.Error,
    V : TtsEngine.Voice
    > : Configurable<S, P>, Closeable {

@mickael-menu
Copy link
Member

I think that looks fine but more importantly I prefer sticking to default options unless they are really asinine.

ktlint_standard_no-wildcard-imports = disabled
ktlint_standard_max-line-length = disabled

ktlint_standard_class-naming = disabled
Copy link
Member

Choose a reason for hiding this comment

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

Did the -naming rules required a breaking update?

.editorconfig Outdated
Comment on lines 20 to 21
ktlint_standard_trailing-comma-on-call-site = disabled
ktlint_standard_trailing-comma-on-declaration-site = disabled
Copy link
Member

Choose a reason for hiding this comment

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

These two triggered too many changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to make that change without some input. From the Kotlin coding conventions:

The Kotlin style guide encourages the use of trailing commas at the declaration site and leaves it at your discretion for the call site.

I'll have it auto-format declaration. If the same is wanted for call, I can run that too.

@qnga
Copy link
Member

qnga commented Nov 18, 2024

I think that looks fine but more importantly I prefer sticking to default options unless they are really asinine.

That's by no means following a different standard, just keeping a bit more freedom. I don't care much though.

@stevenzeck
Copy link
Contributor Author

@mickael-menu @qnga here is a full list of the remaining failures. function-naming is one we will want to keep disabled, as Compose functions generally start with an uppercase letter. The rest, you tell me if they should be fixed.

A (block or EOL) comment inside or on same line after a 'value_parameter' is not allowed. It may be placed on a separate line above. (standard:discouraged-comment-location)

A block comment in between other elements on the same line is disallowed (standard:comment-wrapping)

A comment in a 'type_argument_list' is only allowed when placed on a separate line (standard:discouraged-comment-location)

A comment in a 'value_argument_list' is only allowed when placed on a separate line (standard:discouraged-comment-location)

A comment in a 'value_parameter_list' is only allowed when placed on a separate line (standard:discouraged-comment-location)

A single line block comment after a code element on the same line must be replaced with an EOL comment (standard:comment-wrapping)

Class or object name should start with an uppercase letter and use camel case (standard:class-naming)

Function name should start with a lowercase letter (except factory methods) and use camel case (standard:function-naming)

No comment expected at this location (standard:discouraged-comment-location)

Property name should start with a lowercase letter and use camel case (standard:property-naming)

Property name should use the screaming snake case notation when the value can not be changed (standard:property-naming)

Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you @stevenzeck, I think we can get started with these rules.

@mickael-menu mickael-menu merged commit dc25e7b into readium:develop Nov 19, 2024
4 checks passed
@mickael-menu mickael-menu deleted the ktlint-update branch November 19, 2024 13:24
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.

3 participants