search

open-sauced / hot

🍕The site that recommends the hottest projects on GitHub.
https://hot.opensauced.pizza
MIT License
418 stars 145 forks source link

ci: add husky hooks for formatting #457

Closed diivi closed 1 year ago

diivi commented 1 year ago

What type of PR is this? (check all applicable)

Description

Adds husky hooks to check if a commit message follows conventional commits and to format the code before committing. We could add one for the tests too, but that took a lot of time to pass in my case. They might need some more testing, let me know if something does not conform with the code standards. From the docs - https://typicode.github.io/husky/#/

Related Tickets & Documents

https://github.com/open-sauced/hot/pull/456#issuecomment-1482230991

Mobile & Desktop Screenshots/Recordings

image

Added tests?

Added to documentation?

[optional] Are there any post-deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

netlify[bot] commented 1 year ago

Deploy Preview for hot-sauced-ui ready!

Name Link
Latest commit 86e5fce4e9a3ab6a1d552b137ff64a04b18c3b7c
Latest deploy log https://app.netlify.com/sites/hot-sauced-ui/deploys/641d392d372ecf0008ad44c4
Deploy Preview https://deploy-preview-457--hot-sauced-ui.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

0-vortex commented 1 year ago

Hello @diivi thanks for the contribution, unfortunately, we have already explored husky and regular git hooks and decided against them. Please open an issue or discussion next time you have heavy code suggestions. We don't like to close PRs, but this one going in would create more confusion :<

There are multiple reasons the above is a bad idea, however, the primary push-back is because letting this hook run is that the format command is not magically solving the formatting issues, which are dependent on the users following our standards. That is not fully enforced for linting the commit message, as the contributors should learn how and why to use the npm run push command. Forcing this scenario is raising the entry bar for new contributors even further; we want to have tooling available, but not run it for the user removing their learning curve! 🍕

diivi commented 1 year ago

Ah, that makes sense, my bad. I'll discuss issues on Discord from now on. I agree that the linting commit messages could make contributions from new developers more difficult.

What's wrong with running format though? If it can pass one workflow check client side and can help new devs in making PRs (like the PR mentioned in the issue's description)?

0-vortex commented 1 year ago

Ah, that makes sense, my bad. I'll discuss issues on Discord from now on. I agree that the linting commit messages could make contributions from new developers more difficult.

What's wrong with running format though? If it can pass one workflow check client side and can help new devs in making PRs (like the PR mentioned in the issue's description)?

Eslint doesn't have only auto-format rules. The expected outcome is for new contributors to learn what they are doing and should be doing to get more traction in a project. How does running that on CI and forcing them to re-download changes from the server (that are different than their local files) help?

As it is set up right now, it's the user's responsibility to configure their IDE, most of the time that will pick up our configuration and run it, while some users fail in doing that or even attach their own configuration as global options - for those users, the development flow will break and they will be instructed to do npm run format - if that fails, they have a clear debugging path and also hand held every step of the way.

If we figure out how to magically "fix" all the rules on CI or before a commit, then a user would not know what is even going on, let alone debug the issue. IMHO the best way forward for new contributors is "one step at a time". Also, over-imposing opinionating on users enables them to fast contribute to A specific project, not open source projects in general, which is the idea for this proof of concept repo.

What about the users that make their first commit from the UI? Should we disable their contributions because they don't fit? Since all PRs are squashed anyway, these things don't really matter, ultimately we end up in maintainer technical debt without helping any users learn or contribute faster 😓