Skip to content

Conversation

mnahkies
Copy link
Contributor

@mnahkies mnahkies commented Aug 31, 2025

the com.ncorti.ktfmt.gradle plugin from https://github.com/cortinico/ktfmt-gradle?tab=readme-ov-file#task is already setup, but its not being invoked.

this change adds targets for it to the Makefile and adds a CI step that checks the formatting is correct.

See #697 for the current pending formatting changes.

@mnahkies
Copy link
Contributor Author

issuebot — Any non-trivial git commit must link to a GitHub issue tracking the work. Edit each commit with a tag like "Updates #nn", and update the PR.

IMO this is a trivial change, but happy to open an issue if needed.

I noticed this because I couldn't run the formatting on files I was changing in #695 without muddying the diff with unrelated changes, which is my motivation for proposing to enforce it in CI.

Comment on lines 39 to 40
- name: Run lint
run: make lint-ci
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make this its own workflow (separate yml file) that can run in parallel with building the APKs & running tests. Then it'll fail much faster for developers rather than at the end.

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've reworked it to be a separate job in the same workflow, which should:

  • run in parallel
  • give a separate commit status Android CI / lint
  • whilst keeping the duplicated setup close together

Unsure if there's a better/worthwhile way to DRY the setup steps

Makefile Outdated
Comment on lines 323 to 325
.PHONY: lint-ci
lint-ci: gradle-dependencies ## Check the Android code is formatted
(cd android && ./gradlew ktfmtCheck)
Copy link
Member

Choose a reason for hiding this comment

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

When this fails (especially in CI), it's nice if it can say in the error output what the developer can do to fix it ("run 'make lint' to reformat the source code")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a message in e03a973

Included color code escapes, as I thought the text was a bit subtle / easy to miss otherwise. In two minds about that though as I suspect in CI the color codes will be stripped from the output anyway.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Colors seem to work in CI as well
image

@bradfitz bradfitz force-pushed the mn/ci/check-code-format branch from e03a973 to 109fb34 Compare September 3, 2025 18:02
@mnahkies
Copy link
Contributor Author

mnahkies commented Sep 3, 2025

FYI @bradfitz looks like your squash commit might've dropped the gh actions workflow + hint error message in makefile from the change set

@bradfitz
Copy link
Member

bradfitz commented Sep 3, 2025

@mnahkies, you have multiple overlapping PRs open. I repurposed this one to be just the base part without the GH actions bits. Then the second one can wire it up to PR.

I also renamed the Makefile rules. (and run the fmt commands might myself from the original source to verify it matched)

@bradfitz bradfitz merged commit 981f5e8 into tailscale:main Sep 3, 2025
4 of 5 checks passed
@mnahkies mnahkies deleted the mn/ci/check-code-format branch September 5, 2025 06:54
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.

2 participants