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

fix: migrate to 0.49.0 #40

Merged
merged 1 commit into from
Apr 27, 2023
Merged

fix: migrate to 0.49.0 #40

merged 1 commit into from
Apr 27, 2023

Conversation

berhram
Copy link
Contributor

@berhram berhram commented Apr 23, 2023

Tried to do migration to new version of ktlint сonsidering backward compatibility
https://github.com/pinterest/ktlint/releases/tag/0.49.0

@berhram berhram requested a review from a team as a code owner April 23, 2023 04:34
Copy link
Member

@ghaiszaher ghaiszaher left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Did you try the changes on some repository?

Comment on lines +46 to +48
else
export ANDROID="--code-style=intellij_idea"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Is this the default value? Does it make sense to remove this branch completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends... Right now, it is a real default value. But later they will make another by default - their own ruleset, something like a mix of Kotlin and Android conventions. So it will break interoperability. To prevent that, I have specified the current default for future updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paragraph "'Ktlint Official` code style and renaming of existing code styles" in ktlint changelog you can find in PR desc.

Copy link
Member

Choose a reason for hiding this comment

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

Alright... we can only hope that they don't come up with more breaking changes.

Copy link

@JakubHylaScalable JakubHylaScalable Apr 27, 2023

Choose a reason for hiding this comment

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

@berhram @ghaiszaher Unfortunately that breaks code styling for android. Apparently --code-style=android_studio is incorrect and it should still be --android

Edit: Thanks @ghaiszaher for suggestion of checking .editorconfig configuration - will verify and get back

Choose a reason for hiding this comment

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

Thanks @berhram ! In fact that was problem in our editorconfig. We did not specify ktlint_code_style in .editorconfig at all and relied only on --android parameter (which seemed to work fine pre 0.49.0).

Now after adding

[*.{kt, kts}]
// ...
ktlint_code_style = android_studio

it is totally fine

Copy link
Member

Choose a reason for hiding this comment

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

@berhram trying locally with version 0.49.0, it seems that the flag --code-style=android_studio is ineffective and it only works if .editorconfig is modified with ktlint_code_style = android_studio (then the flag --code-style is not needed at all). Do you know what's up with that?

Copy link
Member

@ghaiszaher ghaiszaher Apr 27, 2023

Choose a reason for hiding this comment

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

Preparing a revert: #41
--android seems to be working fine with latest version.

Update: raising an issue pinterest/ktlint#1982

@ghaiszaher ghaiszaher merged commit cc99a1f into ScaCap:master Apr 27, 2023
ghaiszaher added a commit that referenced this pull request Apr 27, 2023
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.

4 participants