Skip to content
This repository has been archived by the owner on Aug 14, 2022. It is now read-only.

Require code format on commit #115

Closed
thien-do opened this issue Mar 31, 2021 · 9 comments
Closed

Require code format on commit #115

thien-do opened this issue Mar 31, 2021 · 9 comments
Assignees

Comments

@thien-do
Copy link
Collaborator

We use prettier to format our code and we do have a config at https://github.com/moaijs/moai/blob/main/prettier.config.js However, it's totally optional so far, as we can commit without having use it. (Thankfully, our contributors seem to use editors with proper code formatting built-in.)

We should now setup something to run prettier (or at least to check the format) on commit. Maybe use Husky or something similar.

@lqt93
Copy link
Contributor

lqt93 commented Mar 31, 2021

I suggest using husky (https://github.com/typicode/husky) to format code on commit

@thien-do
Copy link
Collaborator Author

Thank you @lqt93 ! Please help me with this

lqt93 added a commit to lqt93/moai that referenced this issue Mar 31, 2021
@lqt93
Copy link
Contributor

lqt93 commented Apr 1, 2021

To use Prettier and Pre-commit hook together, there are some options
I will choose to use first option which uses lint-staged in combination with simple-git-hooks - a lib to help managing git-hooks, and is lighter, simplier to config than husky.

Currently, with this solution, we can also apply "eslint -fix" in pre-commit hook if we have eslint in the future. I think we may also need to apply eslint inside moai for better code format. @thien-do

@thien-do
Copy link
Collaborator Author

thien-do commented Apr 2, 2021

Thank you @lqt93 ! Your comment is great and I totally agree.

Just one thing, is it possible to have this as a check when a contributor creates a PR? Like if a committed file is not formatted, the PR merge should be blocked

@lqt93
Copy link
Contributor

lqt93 commented Apr 2, 2021

I know it is possible but at the moment, I still don't have a specific solution for that. Need to research more.

@ZeroX-DG
Copy link
Contributor

ZeroX-DG commented Apr 3, 2021

I suggest using github action for CI (just because it's native to github), but feel free to use other services such as travisCI. After that we can setup a status check before merging rule for the master branch. This way the tests & formatting jobs will be run on the CI. The PR can only be merged if the CI is passed.

https://docs.github.com/en/github/administering-a-repository/managing-a-branch-protection-rule

@thien-do
Copy link
Collaborator Author

thien-do commented Apr 3, 2021

I think we will go with GitHub actions :D The cost of bringing another CI service is just too much for this project :( Even with someone set it up for us, I'd think the future maintainers would like to continue with that

lqt93 added a commit to lqt93/moai that referenced this issue Apr 4, 2021
@lqt93
Copy link
Contributor

lqt93 commented Apr 4, 2021

husky updated to 6.0.0 and lint-staged abandoned simple-git-hooks: lint-staged/lint-staged#960
so, back to use husky for setting up git hooks.

lqt93 added a commit to lqt93/moai that referenced this issue Apr 4, 2021
lqt93 added a commit to lqt93/moai that referenced this issue Apr 4, 2021
lqt93 added a commit to lqt93/moai that referenced this issue Apr 4, 2021
@thien-do
Copy link
Collaborator Author

Ok I'll close this in light of @lqt93 's excellent #135 . However it's worth to note that we won't run or enforce code format on commit, but as a check of the PR. Also this check would scan the whole repo (which is still quite fast) instead of just staged files.

It's not as good as a commit hook on staged files, yes, but it significantly simplified our setup, which is some thing I value a lot. For our current and near-future codebase size, I think we're good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants