Skip to content

feat: remove pre-commit#8406

Merged
michalsn merged 2 commits intocodeigniter4:developfrom
michalsn:feat/remove-pre-commit
Jan 9, 2024
Merged

feat: remove pre-commit#8406
michalsn merged 2 commits intocodeigniter4:developfrom
michalsn:feat/remove-pre-commit

Conversation

@michalsn
Copy link
Copy Markdown
Member

@michalsn michalsn commented Jan 6, 2024

Description
This PR removes pre-commit and all related code.

While the idea of pre-commit was a good one, IMO today it brings developers more problems than good. The number of errors that can occur, even if we haven't touched some files, is simply too high. This can cause many users (even experienced ones) to abandon the changes they are preparing and not send a PR.

A side effect may be that maintainers will have more work to do in handling PRs, but it's worth the risk.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

Copy link
Copy Markdown
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I agree to this change. In fact, when committing in vscode, I always use the --no-verify option and let Github Actions do the job.

Copy link
Copy Markdown
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Thank you for this. My only other thought was that we had a message reminding about the commands they can run on commit but that's not necessary and can be done later if desired.

@sfadschm
Copy link
Copy Markdown
Contributor

sfadschm commented Jan 6, 2024

Regarding the extra work for the maintainers I believe to have seen bots respond to PRs in other repositories, where they automatically call out the failed CI Checks and give hints on how to fix.

Like:
"Code style check failed. Please run php-cs-fixer..."

If this is possible it could replace the very frequent comment of "Please fix the coding style", "Commits must be signed" and so on.

@kenjis
Copy link
Copy Markdown
Member

kenjis commented Jan 7, 2024

I use it. But if you all agree, I don't disagree.
But the current checks are so basic that I don't see why even @paulbalandan is disabling it.

kenjis
kenjis previously approved these changes Jan 7, 2024
@kenjis
Copy link
Copy Markdown
Member

kenjis commented Jan 7, 2024

@michalsn Can we keep pre-commit and setup.sh?
If they are not installed automatically, there is no harm.

@kenjis kenjis dismissed their stale review January 7, 2024 00:50

I hope to keep them.

@michalsn
Copy link
Copy Markdown
Member Author

michalsn commented Jan 7, 2024

@kenjis So you only want to remove it from the composer.json so people can apply it manually? Since you seem to actively using it - sure.

But the current checks are so basic that I don't see why even @paulbalandan is disabling it.

For a couple of days, we had our checks failing on the develop branch. I also remember a few cases when errors pop up when we change unrelated things. All these cases would prevent users from sending a PR. Not everyone knows how to skip verification or remove pre-commit entirely. At this point, this seems to be an issue for contributors.

@kenjis
Copy link
Copy Markdown
Member

kenjis commented Jan 7, 2024

@michalsn Yes, thanks.
The recent php-cs-fixer errors are caused by the update of cs-fixer.
Unfortunately it happens sometime these days.

I agree with making the hook opt-in.

@michalsn michalsn merged commit d64fdb5 into codeigniter4:develop Jan 9, 2024
@michalsn
Copy link
Copy Markdown
Member Author

michalsn commented Jan 9, 2024

Thanks, everyone!

@michalsn michalsn deleted the feat/remove-pre-commit branch February 21, 2026 20:14
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.

5 participants