Skip to content
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

add 'postinstall' to enable hooks after install #32

Merged
merged 4 commits into from Nov 21, 2023
Merged

Conversation

kennytram
Copy link
Contributor

@kennytram kennytram commented Oct 29, 2023

Description

Enable Git hooks for Husky immediately after yarn install husky's doc

For current developers, please run yarn husky install to ensure that Husky functions correctly.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • After yarn husky install
    -- Intentionally made lint mistakes in some files after yarn husky install
    -- Detected by Husky after push

  • Before yarn husky install
    -- Intentionally made lint mistakes before yarn husky install
    -- Not detected by Husky after push

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@cherylli
Copy link
Collaborator

not working for me
image

@cherylli
Copy link
Collaborator

So I fixed it now and working, it's a windows issue (for me at least)

I followed this
typicode/husky#850 (comment)

@smurph7894
Copy link
Contributor

smurph7894 commented Nov 1, 2023

Just fyi, the husky script does not work with Github Desktop but I am able to do it using gitbash.

smurph7894
smurph7894 previously approved these changes Nov 1, 2023
Copy link
Contributor

@smurph7894 smurph7894 left a comment

Choose a reason for hiding this comment

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

see FYI comment but otherwise is working for me.

@cherylli cherylli mentioned this pull request Nov 3, 2023
12 tasks
@cherylli
Copy link
Collaborator

cherylli commented Nov 3, 2023

So current husky setup is not quite working for me, it goes like this,
I push, husky runs automatically, but the code BEFORE formatting was pushed (linting seems to be ignored - I don't see any errors when it had errors when I manually run yarn lint)
so I had to do another commit for the formatted code

So the correct workflow should be

  1. issue push command
  2. husky runs
  3. push FORMATTED code

is this happening to anyone else? I haven't looked it up yet but I was wondering if it was just me or not

@kennytram
Copy link
Contributor Author

kennytram commented Nov 3, 2023

So current husky setup is not quite working for me, it goes like this, I push, husky runs automatically, but the code BEFORE linting was pushed so I had to do another commit for the formatted code

So the correct workflow should be

  1. issue push command
  2. husky runs
  3. push FORMATTED code

is this happening to anyone else? I haven't looked it up yet but I was wondering if it was just me or not

Pre-commit - Detect and format files after commit
Pre-push - Detect and format files after push

Although, it does seem strange why Husky is only doing changes after a push when it's supposed to be "pre-push". I'll look it up this weekend if no one else does.

edit: "When you run the git push command, Git triggers any pre-push hooks that are installed. These hooks are intended to inspect the changes that are about to be pushed and possibly prevent the push if something isn't right. However, at this stage, the changes have already been committed to your local repository and are ready to be pushed. If your pre-push hook makes additional changes to the code (like reformatting it), these changes will not be included in the push because they haven't been committed yet"

It seems like we would need to enter additional commands for it to work as you intended.
We can do something like this:

yarn format
yarn lint
git add . {note: if we want to add staged files only, we will need to install lint-staged}
git commit -m "formatted code"

This will make two separate commits in one single push command.

What do you think?

@cherylli
Copy link
Collaborator

cherylli commented Nov 3, 2023

What do you think?

that's basically what I have to do now, but manually, but we shouldn't have to make an extra commit to include husky changes

if you look at this as an example, I basically have to do 2 commits every time I push, and one just for the husky changes
dev...feature/sprints-endpoints

Is there any reason we can't use a pre commit hook? I've used pre commit hooks for format in all my other projects and no issues

@kennytram
Copy link
Contributor Author

What do you think?

that's basically what I have to do now, but manually, but we shouldn't have to make an extra commit to include husky changes

if you look at this as an example, I basically have to do 2 commits every time I push, and one just for the husky changes

dev...feature/sprints-endpoints

Is there any reason we can't use a pre commit hook? I've used pre commit hooks for format in all my other projects and no issues

Dan doesn't like it 😄

"Pre-commit would get too annoying" in his exact words.

@cherylli
Copy link
Collaborator

cherylli commented Nov 3, 2023

What do you think?

that's basically what I have to do now, but manually, but we shouldn't have to make an extra commit to include husky changes
if you look at this as an example, I basically have to do 2 commits every time I push, and one just for the husky changes
dev...feature/sprints-endpoints
Is there any reason we can't use a pre commit hook? I've used pre commit hooks for format in all my other projects and no issues

Dan doesn't like it 😄

"Pre-commit would get too annoying" in his exact words.

he's not going to commit or push this repo much. double commits annoys me and clogging the commit history 😂

What does everyone else in the team think?

@kennytram
Copy link
Contributor Author

kennytram commented Nov 3, 2023

edit: Nvm I realized my solution isn't the correct approach for pre-push. I reverted my recent changes.

@kennytram kennytram force-pushed the feature/husky branch 2 times, most recently from 085dde2 to d883e97 Compare November 3, 2023 08:09
@jenny-alexander
Copy link
Contributor

I haven't tried any of this yet - but agree that it should be a pre-commit hook and we shouldn't push two commits for this.
If we want to keep our git commit history clean"er", it's possible to do a git rebase.

@Dan-Y-Ko
Copy link
Collaborator

Dan-Y-Ko commented Nov 7, 2023

What do you think?

that's basically what I have to do now, but manually, but we shouldn't have to make an extra commit to include husky changes

if you look at this as an example, I basically have to do 2 commits every time I push, and one just for the husky changes dev...feature/sprints-endpoints

Is there any reason we can't use a pre commit hook? I've used pre commit hooks for format in all my other projects and no issues

If you use a pre commit hook, literally every time you try to commit, you will have to fix any linting errors. This honestly is a just a huge hassle. For me personally, I always do multiple commits and then when I'm done, I'll push it up to the repo and before I push, I do a lint/format. I like having the check in a pre-push in case I or someone else forgets to do that. Having to do 1 extra commit is way better tradeoff for me than having to fix stuff every time I commit (I'm not really sure what you mean about double commit, it should only be 1 extra). That being said that part is not relevant to the husky workflow issue and can be tweaked for each team. When I tried to set it up on the fe, it didn't matter what I used, it wasn't working for other people so yea.

@jenny-alexander jenny-alexander added the enhancement New feature or request label Nov 20, 2023
Copy link
Collaborator

@cherylli cherylli left a comment

Choose a reason for hiding this comment

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

works for me, thanks for fixing this

Copy link
Contributor

@jenny-alexander jenny-alexander left a comment

Choose a reason for hiding this comment

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

It works for me after I set the pre-commit script to executable. Approved!

@kennytram kennytram merged commit 9686b4f into dev Nov 21, 2023
1 check passed
@kennytram kennytram deleted the feature/husky branch November 21, 2023 23:56
@jenny-alexander jenny-alexander restored the feature/husky branch November 22, 2023 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants