Skip to content

Conversation

@CDRussell
Copy link
Member

Task/Issue URL: https://app.asana.com/0/488551667048375/1176263457388604
Tech Design URL:
CC:

Description:
Adds ability to automate code formatting checks using ktlint.

Steps to test this PR:

  1. Install develop first and run the app. Then install this version to ensure no migration issues from the change (a DB entity class was renamed because it violated the code style)
  2. Run ./gradlew ktlint and verify there are no reported violations.
  3. Deliberately add a violation to a Kotlin class (e.g., add a bunch of useless new lines) and run ktlint again. This time, verify the build fails and reports the issue.

ℹ️ Note, the Bitrise checks do not yet enforce these rules. There is a new workflow ready to go but I can't switch to using that until this is merged. Once this is merged, I'll configure Bitrise to start failing on new violations.


Internal references:

Software Engineering Expectations
Technical Design Template

@malmstein
Copy link
Contributor

@CDRussell thanks for submitting this, it's a great addition. I've got a couple of questions:

  • Are there any different rules between this and our current style guide? If so, can we have that updated too?
  • Did you look into enforcing these rules before pushing the code too? Other projects have something like that and could save up a lot of time

@CDRussell
Copy link
Member Author

CDRussell commented May 18, 2020

Are there any different rules between this and our current style guide? If so, can we have that updated too?

It should be mostly the same, though i'm sure we'll find some further tweaks required at some point. As a result of the commits tidying up the existing code, there should be no changes made if you format the entire codebase in Android Studio now. And so any new detections should be valid.

The one thing that stood out was that it detected a lot of files didn't end with a new line, which is recommended. I don't know if there's a way to get Android Studio to enforce this too for an even quicker alert.

Did you look into enforcing these rules before pushing the code too

No, I considered it but wanted to just get the minimum done to start getting value from it. Agree that it's a good addition though, and happy to iterate on it to make it a strong requirement / prerequisite to format the code locally with a git hook etc...

@malmstein
Copy link
Contributor

Thanks for the clarification @CDRussell. I can't find that check either (it exists for Java but not for Kotlin) so I agree that this is a good solution for now

@CDRussell
Copy link
Member Author

Thanks for checking. If it becomes an issue causing lots of noise, we can either:

  1. enforce formatting before pushing [preferable]
  2. ignore that rule

@malmstein malmstein assigned malmstein and unassigned subsymbolic May 19, 2020
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

Discussed this online and after testing the ktlint warnings and formatting this is good to go!

@CDRussell CDRussell merged commit db72852 into develop May 19, 2020
@CDRussell CDRussell deleted the feature/craig/lint_checking branch May 19, 2020 11:26
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