search

DroidKaigi / conference-app-2023

The Official Conference App for DroidKaigi 2023
Apache License 2.0
647 stars 206 forks source link

[Build] Make be easy code formatting #40

Open tomoya0x00 opened 1 year ago

tomoya0x00 commented 1 year ago

We've already checked the code format of PRs by Format GitHub Action. But the contributors may forget to format their own code before creating their PRs. It would be better to introduce a way to avoid forgetting it.

tomoya0x00 commented 1 year ago

ref: the result of Format GitHub Action

https://github.com/DroidKaigi/conference-app-2023/pull/39

image

tomoya0x00 commented 1 year ago

How about introducing Spotless plugin for Gradle or/and suggesting installing Spotless Gradle Idea plugin in the README.md?

tomoya0x00 commented 1 year ago

@takahirom Please let me know if what I am trying to do is out of line with your intentions.

imsakuu commented 1 year ago

But the contributors may forget to format their own code before creating their PRs. It would be better to introduce a way to avoid forgetting it.

I think that makes sense. For example, how about using pre-commit in Git hooks? In this case, we would need to have a script to set up the pre-commit and have the contributor run it themselves.

tomoya0x00 commented 1 year ago

Thanks for your comment!

For example, how about using pre-commit in Git hooks?

Yeah, it is one of the solutions. I'm not familiar with Git hooks, so I didn't give it as an option.

I wonder is it possible to apply pre-commit automatically when a contributor clones this repository?

imsakuu commented 1 year ago

I don't know how to apply them automatically, but it is possible to provide a simple way.

step1: write our-pre-commit

./gradlew spotlessCheck
...

step2: write our-setup-script.sh

...
cp our-pre-commit .git/hooks/pre-commit

step3: clone and run the script (This is all the contributor has to do.)

$ git clone ...
$ our-setup-script.sh

So, we can introduce in README.md or CONTRIBUTING.md that the setup script needs to be run after git clone.

jmatsu commented 1 year ago

Only trusted projects can tweak gradlew to install git-hooks automatically. I guess it might sound hacky though.

By the way, a git-hook is powerful and I believe it will be a great help in fact but it works only if configured properly. And also, it can be skipped by adding --no-verify option and it may prevent commit and/or push without their notices. This means a git-hook are not a solution but just a help. A similar solution is inserting a code format action to a build task graph, although it can also be no-op if they do not execute a build task.

Anyway, I'm not against you and hook-based solutions. I'd like to say the hook-based approach may be insufficient.

Okay, let's go back to consider the point of what are acceptable code changes. AFAIK, the previous format workflow sounds slightly strict for me because it has alerted when the changes break code formatting rules anyway. However, we don't have to reject the changes immediately when all of broken code styles in the changes can be fixed by auto-corrections. Because, the changes will be acceptable by applying the fix somehow after reviews are approved but before merging.

So I'd love to suggest that we should do the following two things:

takahirom commented 1 year ago

I'm not particular about using KtLint; any other method is fine as well.

Regarding the hook, I don't think it would be great if the experience becomes waiting for 30 seconds every time you commit. If it can be done within 10 seconds even when the project gets somewhat larger(About the size of the repository at DroidKaigi in previous years), that might be acceptable. Personally, I think it might be better to use CI to format on pull requests if possible, but I'm not sure if it's possible to modify forked repositories.

takahirom commented 1 year ago

However, I have never developed while formatting in Hook properly, so for me, there is no problem to try it in Hook and change it if there is a problem.

tomoya0x00 commented 1 year ago

I think it might be better to use CI to format on pull requests if possible, but I'm not sure if it's possible to modify forked repositories.

I have discussed this with ChatGPT(GPT-4) and it seems difficult.

By the way, is it too strict to make gradlew assemble fail if there is a code format violation in a PR? We’ve already set up the Spotless plugin for Gradle in our project, and gradlew build fails if there is a violation. Similarly, I was wondering if it would be a good(or bad?) idea to make gradlew assemble fail and suggest that contributors run gradlew spotlessApply.

tomoya0x00 commented 1 year ago

I think the code formatting can be applied to our main branch after a PR is merged into our repository, but it is going to be a bit difficult to see the commit log for the main branch. Also, I'm a bit afraid of automatically changing the main branch.

jmatsu commented 1 year ago

By the way, is it too strict to make gradlew assemble fail if there is a code format violation in a PR? We’ve already set up the Spotless plugin for Gradle in our project, and gradlew build fails if there is a violation. Similarly, I was wondering if it would be a good(or bad?) idea to make gradlew assemble fail and suggest that contributors run gradlew spotlessApply.

A similar solution is inserting a code format action to a build task graph, although it can also be no-op if they do not execute a build task. from https://github.com/DroidKaigi/conference-app-2023/issues/40#issuecomment-1513572112

Same here though, I'm afraid contributors can avoid it and editing from GitHub web UI cannot follow it.

I think the code formatting can be applied to our main branch after a PR is merged into our repository, but it is going to be a bit difficult to see the commit log for the main branch. Also, I'm a bit afraid of automatically changing the main branch.

We can run a workflow after the review is approved before merging.

takahirom commented 1 year ago

If we use pull_request_target, maybe we can use the privileges of the forked repository, so we can commit over format?

tomoya0x00 commented 1 year ago

Summary of what we discussed in our online meeting today:

We think it would be possible to detect when a PR is approved and automatically merge it into the main branch of our Repository after code formatting.

The important points are the following.

First, let’s change our GitHub actions. If a PR has a code format violation, then comment “Please run ./gradlew spotlessKotlinApply”.

takahirom commented 1 year ago

For reference, I found compose-samples push the formatted commit to the pull request to format. 👀 https://github.com/android/compose-samples/blob/f87eca899d608be5d5f6542cf46e225d65dc84bb/.github/workflows/main.yml#L37