-
Notifications
You must be signed in to change notification settings - Fork 732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: deps husky and commitlint #19
chore: deps husky and commitlint #19
Conversation
@balaji-sivasakthi I appreciate your effort in keeping the project and commit messages tidy. However, upon testing the implementation, I discovered that the Also, is it expected that i run Getting the following warning upon committing.
Please let me know your thoughts on this matter, and if you agree with these points. I would love to learn more about this. |
@wajeshubham Thank you! Husky will handle the pre-commit on its own, so there's no need to execute Sorry for the trouble, It was an egde case in mac/Linux
To prevent this issue, we can allow permission to the chmod ug+x .husky/* This command grants execute ( {
"prepare": "husky install | chmod ug+x .husky/*"
} |
@balaji-sivasakthi So, how about adding #!/usr/bin/env sh
. "$(dirname -- "$0")/_/husky.sh"
yarn prepare # prepare husky hooks
npx --no-install commitlint --edit Also, NOTE: As this PR has changes that are involved in the contribution flow we also need to update the Contribution guide according to this new convention of commit messages. |
"prepare": "husky install | chmod ug+x .husky/*" Yes, this script would break on Windows. However, it can be sorted out by using cross-spawn for executing the prepare script. import { spawnSync } from 'child_process';
import os from 'os';
// Function to run commands depending on the OS
const runCommand = (command, args) => {
const isWindows = os.platform() === 'win32';
const shell = isWindows ? true : false;
const commandArgs = isWindows ? ['/C', command, ...args] : [...args];
const result = spawnSync(isWindows ? 'cmd' : command, commandArgs, {
stdio: 'inherit',
shell,
});
return result.status;
};
// Function to prepare Husky
const prepareHusky = () => {
// Run husky install
const huskyInstallStatus = runCommand('npx', ['husky', 'install']);
// Set permissions for husky on macOS/Linux
if (os.platform() !== 'win32') {
const setPermissionsStatus = runCommand('chmod', ['+x', '.husky/*']);
if (setPermissionsStatus !== 0) {
console.error('Setting permissions failed');
process.exit(1);
}
}
// Exit with appropriate status code based on husky install status
if (huskyInstallStatus === 0) {
console.log('Husky preparation executed successfully!');
process.exit(0);
} else {
console.error('Husky installation failed');
process.exit(1);
}
};
prepareHusky()
Ok, Sure I will add all necessary docs. |
@balaji-sivasakthi script is failing in the macOS on running # Error:
husky - Git hooks installed
chmod: .husky/*: No such file or directory # I have .husky folder. Testing your branch only
Setting permissions failed
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command. Also, seems like an extra boilerplate to just lint the commit, which no doubt is a good practice. However, it would be great if we consider and test all the edge cases before merging. Thank you! 😀 |
9926efa
to
492c8cf
Compare
492c8cf
to
6ebf23a
Compare
I've tested it in GitHub Codespace, and it works well now. |
@wajeshubham I'm also updated README.md and CONTRIBUTOR.md for conventional commit message format. Please take a moment to look at this. |
d3723d5
to
34ce557
Compare
@balaji-sivasakthi thanks for the PR, I will review it thoroughly and provide the approval |
@wajeshubham Is this ok? |
Yes @balaji-sivasakthi we are reviewing it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some comments after reviewing the changes, please go through them
@jwala-anirudh Please check the recent changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Related issue:-
Issue #16
Changes Made:-
Additional Context:-