-
Notifications
You must be signed in to change notification settings - Fork 199
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
Integrate ktlint and fix codestyle violations #115
Conversation
Thanks for this. We are going to solidify what style we are looking for and check that the rules in KtLint will work |
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.
Not 100% clear on how things are synced between ktlint and ide settings but if that gradle plugin is doing it I can trust it. Just some nits in the details.
@@ -15,22 +15,21 @@ | |||
android:layout_width="wrap_content" | |||
android:layout_height="32dp" | |||
android:gravity="center_vertical" | |||
android:text="@string/breed" |
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.
FYI a nice way to keep lint happy without needing to make this a string resource is to use tools:text
. This will show it in the layout preview only. The other advantage of that is we won't accidentally leave bad initial text in-place if something fails to initialize in code.
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 thought about this, but idk if in the case of failing to initialize blank is better than a default like this. I could see either way
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.
Yeah. Not a huge deal since it's just sample code anyway. Don't need to dig to hard on Android best-practices when most people's focus will be on KMP and iOS.
}) | ||
viewModel.breedLiveData.observe( | ||
this, | ||
Observer { itemData -> |
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.
Not for this PR, but I think there's finally a ktx thing for this somewhere so we can drop the Observer
and do a trailing lambda. Might be misremembering though.
They are synced but just a subset, there is an issue to address that. |
Thanks for the extra insight @romtsn! |
I saw somewhere you were not happy with the inconsistent code style, so decided to fix that :) Let me know if something is contradicting your internal guidelines.