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

Install pre-commit script should overwrite the file. #308

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kopepasah
Copy link
Contributor

When running the pre-commit install script from wp-dev-lib, the pre-commit file in .git/hooks should be overwritten, if it exists. Currently, the script just fails and this is troublesome when trying to run the install script on the post-install-cmd script for Composer.

@kopepasah
Copy link
Contributor Author

Another option would be to check if the file already exists, but — IMO — running this script should take precedence and just overwrite the file.

@kasparsd
Copy link
Contributor

kasparsd commented Dec 9, 2019

Projects can have multiple pre-commit scripts for things outside of what dev-lib does so we suggest ways for adding the dev-lib's pre-commit alongside existing scripts using husky (npm) and composer-git-hooks (PHP) in this section of readme.md. Would one of the suggested approaches work for your solution @kopepasah?

@kopepasah
Copy link
Contributor Author

Sure, any one of those method will work, but it still seems odd that a user can run this script (with the expectation that a pre-commit hook will install), but nothing gets installed. At that point, the user may not know what to do (depending on their level of expertise).

Is there a reason we do not want to overwrite the pre-commit script if a user invokes this install script?

@kasparsd
Copy link
Contributor

it still seems odd that a user can run this script (with the expectation that a pre-commit hook will install), but nothing gets installed.

💯Absolutely!

How about we add a prompt to ask if the file should be overwritten if it already exists? Like you said -- the purpose of this script is to setup pre-commit and it doesn't make sense that it silently fails. It should either report an error that a file already exists or ask if it should override it anyways.

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

Successfully merging this pull request may close these issues.

2 participants