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

Added git hook #723

Merged
merged 4 commits into from
Jan 6, 2022
Merged

Added git hook #723

merged 4 commits into from
Jan 6, 2022

Conversation

Rishabhraghwendra18
Copy link
Contributor

@Rishabhraghwendra18 Rishabhraghwendra18 commented Nov 11, 2021

issue: #701
Added git hook using Husky package .

Since this PR doesn't have package.json file , so to make husky work run the below command:
yarn add husky --dev
and paste this in package.json file:

{
  "scripts": {
    "prepare": "husky install"
  }
}

Husky documentation

lib/wallet-account.js Outdated Show resolved Hide resolved
@volovyks volovyks mentioned this pull request Nov 16, 2021
@volovyks
Copy link
Collaborator

@Rishabhraghwendra18 husky dep is added, now you can use it in this PR.

@volovyks volovyks linked an issue Nov 19, 2021 that may be closed by this pull request
@Rishabhraghwendra18
Copy link
Contributor Author

Rishabhraghwendra18 commented Nov 22, 2021

@volovyk-s I have already used the husky dep in this PR . I just made the PR for config files required for husky dep only.

@volovyks
Copy link
Collaborator

@Rishabhraghwendra18 just run the following commands:

git checkout master
git pull
git checkout <your_branch>
git merge master
git push

It will make this PR a real working one.

@Rishabhraghwendra18
Copy link
Contributor Author

@Rishabhraghwendra18 just run the following commands:

git checkout master
git pull
git checkout <your_branch>
git merge master
git push

It will make this PR a real working one.

may we directly pull in our branch & then push like this:

(directly pulling in our current branch)
git pull
git push

@volovyks
Copy link
Collaborator

@Rishabhraghwendra18 just run the following commands:

git checkout master
git pull
git checkout <your_branch>
git merge master
git push

It will make this PR a real working one.

may we directly pull in our branch & then push like this:

(directly pulling in our current branch)
git pull
git push

You need changes from the master branch. I have added husky to master.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2021

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this will be closed in another 7 days.

@github-actions github-actions bot added the Stale label Dec 3, 2021
@Rishabhraghwendra18
Copy link
Contributor Author

@volovyk-s everything is up-to-date in this PR . I have ran your git commands it's showing everything is up to date.

Sorry for late reply I thought I have commented about this

@volovyks
Copy link
Collaborator

volovyks commented Dec 3, 2021

@Rishabhraghwendra18 yes, the master is merged now.
But the solution is not working. yarn build was not executed before the commit.
Please, add all the needed setup and a small doc in Readme on how to use it if it does not work automatically.

@github-actions github-actions bot removed the Stale label Dec 4, 2021
@Rishabhraghwendra18
Copy link
Contributor Author

Rishabhraghwendra18 commented Dec 7, 2021

@Rishabhraghwendra18 yes, the master is merged now. But the solution is not working. yarn build was not executed before the commit. Please, add all the needed setup and a small doc in Readme on how to use it if it does not work automatically.

It's working on my machine . Make sure you have followed these steps:

  1. paste this script in package.json file:
{
  "scripts": {
    "prepare": "husky install"
  }
}
  1. run yarn install in the directory to install husky module.

To test whether husky is working or not , add exit 1 after yarn build command in .husky/pre-commit file

I don't think it's need to add it about in readme . User just have to commit everything else will be taken care of by husky . If then also you think we should add about it then no problem I can add about it

@github-actions
Copy link
Contributor

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this will be closed in another 7 days.

@github-actions github-actions bot added the Stale label Dec 15, 2021
@Rishabhraghwendra18
Copy link
Contributor Author

@Rishabhraghwendra18 yes, the master is merged now. But the solution is not working. yarn build was not executed before the commit. Please, add all the needed setup and a small doc in Readme on how to use it if it does not work automatically.

It's working on my machine . Make sure you have followed these steps:

  1. paste this script in package.json file:
{
  "scripts": {
    "prepare": "husky install"
  }
}
  1. run yarn install in the directory to install husky module.

To test whether husky is working or not , add exit 1 after yarn build command in .husky/pre-commit file

I don't think it's need to add it about in readme . User just have to commit everything else will be taken care of by husky . If then also you think we should add about it then no problem I can add about it

@volovyk-s is it now working ?

@volovyks
Copy link
Collaborator

@Rishabhraghwendra18 I can not edit your PR and I can not merge it in a non-working state. Please, add all the necessary code and configs first.

@github-actions github-actions bot removed the Stale label Dec 16, 2021
@Rishabhraghwendra18
Copy link
Contributor Author

Rishabhraghwendra18 commented Dec 16, 2021

@Rishabhraghwendra18 I can not edit your PR and I can not merge it in a non-working state. Please, add all the necessary code and configs first.

@volovyk-s ok.. but I have enabled allow Allow edits by maintainers checkbox . No problem I re-write the steps :
1st Step: Run any of these commands (as per the yarn whether it's Yarn 1 or Yarn 2) in the project root directory:

npx husky-init && yarn              # Yarn 1
yarn dlx husky-init --yarn2 && yarn # Yarn 2

This will install and create a .husky directory , having a default script which will run yarn test ,in the root of the project.

2nd Step: Now commit any changes by running:

 git add .
 git commit -m "<your message here>" 

You will see the tests begin running when you commit. Hence proves that husky is successfully installed

I hope this will work now

@github-actions
Copy link
Contributor

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this will be closed in another 7 days.

@github-actions github-actions bot added Stale and removed Stale labels Dec 24, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2022

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this will be closed in another 7 days.

@github-actions github-actions bot added the Stale label Jan 1, 2022
@volovyks
Copy link
Collaborator

volovyks commented Jan 4, 2022

I have added the "prepare" script and fixed the hook. We should not add all the files, only build artifacts.

@volovyks volovyks requested review from ailisp and itegulov January 4, 2022 17:08
@github-actions github-actions bot removed the Stale label Jan 5, 2022
@Rishabhraghwendra18
Copy link
Contributor Author

I have added the "prepare" script and fixed the hook. We should not add all the files, only build artifacts.

ok.. Thanks

@itegulov
Copy link
Contributor

itegulov commented Jan 5, 2022

@volovyk-s have you actually tested it? I have just tried it out myself and even though the pre-commit hook is being run, the modified files are not being staged for the commit. In other words, if you change src/connection.ts and run git commit it successfully generates lib/connection.js and lib/connection.d.ts, but these files do not become a part of the commit that is being created (i.e. git add lib has no effect). I have googled the issue and it seems to be a common misconception about how pre-commit hooks:

As the answers suggest, there are seem to be ways to mitigate this by using a .commit file and creating a separate post-commit hook that amends the commit, but I think it will be too much trouble for such a minute thing. What do you think if instead we were just to print a red text prompting user to re-run git commit manually? The current commit would be aborted ofc.

@ailisp
Copy link
Member

ailisp commented Jan 5, 2022

I think @itegulov 's point makes sense, ask the user to git commit again is a reasonable workflow while not making the hook over complex

@volovyks
Copy link
Collaborator

volovyks commented Jan 5, 2022

@itegulov @ailisp that is strange. It works for me just fine. Here is the output from my console (sorry, it's a bit long).

> ~/P/N/tmp git clone git@github.com:Rishabhraghwendra18/near-api-js.git
Cloning into 'near-api-js'...
remote: Enumerating objects: 23967, done.
remote: Counting objects: 100% (1712/1712), done.
remote: Compressing objects: 100% (601/601), done.
remote: Total 23967 (delta 1262), reused 1473 (delta 1099), pack-reused 22255
Receiving objects: 100% (23967/23967), 8.48 MiB | 2.02 MiB/s, done.
Resolving deltas: 100% (16728/16728), done.

⋊> ~/P/N/tmp cd near-api-js/                                                                                                                           15:50:26

⋊> ~/P/N/t/near-api-js on master ◦ git checkout gitHooks                                                                                               15:50:31
Branch 'gitHooks' set up to track remote branch 'gitHooks' from 'origin'.
Switched to a new branch 'gitHooks'> ~/P/N/t/near-api-js on gitHooks ◦ yarn                                                                                                              15:50:42
yarn install v1.22.17
warning ../../../../package.json: No license field
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
warning "danger > @octokit/rest > @octokit/plugin-request-log@1.0.4" has unmet peer dependency "@octokit/core@>=3".
[4/4] Building fresh packages...
$ not-in-install && (yarn build && yarn browserify) || in-install
$ husky install
husky - Git hooks installed
Done in 4.65s.

⋊> ~/P/N/t/near-api-js on gitHooks ◦ yarn prepare                                                                                                      15:50:49
yarn run v1.22.17
warning ../../../../package.json: No license field
$ husky install
husky - Git hooks installed
Done in 0.11s.

⋊> ~/P/N/t/near-api-js on gitHooks ◦ code .                                                                                                            15:50:53

⋊> ~/P/N/t/near-api-js on gitHooks ◦ git diff                                                                                                          15:50:58
diff --git a/src/account.ts b/src/account.ts
index 25f19608..8f364b21 100644
--- a/src/account.ts
+++ b/src/account.ts
@@ -215,6 +215,7 @@ export class Account {
      */
     protected signAndSendTransaction(receiverId: string, actions: Action[]): Promise<FinalExecutionOutcome>
     protected signAndSendTransaction(...args: any): Promise<FinalExecutionOutcome> {
+        console.log("hey");
         if(typeof args[0] === 'string') {
             return this.signAndSendTransactionV1(args[0], args[1]);
         } else {

⋊> ~/P/N/t/near-api-js on gitHooks ⨯ git add --all                                                                                                     15:51:22

⋊> ~/P/N/t/near-api-js on gitHooks ⨯ git status                                                                                                        15:51:28
On branch gitHooks
Your branch is up to date with 'origin/gitHooks'.

Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   src/account.ts

⋊> ~/P/N/t/near-api-js on gitHooks ⨯ git commit -m "test commit"                                                                                       15:51:30
yarn run v1.22.17
warning ../../../../package.json: No license field
$ yarn compile
warning ../../../../package.json: No license field
$ tsc -p ./tsconfig.json
Done in 4.92s.
[gitHooks 9b710846] test commit
 2 files changed, 1 insertion(+)

⋊> ~/P/N/t/near-api-js on gitHooks ↑ git status                                                                                                        15:51:44
On branch gitHooks
Your branch is ahead of 'origin/gitHooks' by 1 commit.
  (use "git push" to publish your local commits)

nothing to commit, working tree clean

⋊> ~/P/N/t/near-api-js on gitHooks ↑ git log                                                                                                           15:51:53
commit 9b7108464db607179e2e9396ee56a8c6c6ec878e (HEAD -> gitHooks)
Author: Serhii Volovyk <sergeyvolovyk@gmail.com>
Date:   Wed Jan 5 15:51:39 2022 +0200

    test commit

commit bf2ce5e027a3bdfa0fc15eb1419430967b98f771 (origin/gitHooks)
Author: Serhii Volovyk <sergeyvolovyk@gmail.com>
Date:   Tue Jan 4 19:00:56 2022 +0200

    husky prepare script added, pre-commit hook fixed

commit 7bd727836f777d5b0f362b63c0d620f0ecc9499d
Merge: f950343d 5c2ea7b4
Author: Rishabhraghwendra18 <rishabhraghwendra2002@gmail.com>
Date:   Tue Nov 23 16:33:35 2021 +0530

    Merge branch 'master' of https://github.com/near/near-api-js into gitHooks

commit 5c2ea7b41e9be97394ccf15ef7926c592904fa82
Author: Serhii Volovyk <SergeyVolovyk@gmail.com>
Date:   Fri Nov 19 17:39:07 2021 +0100

    husky dep added (#727)

commit 90d58dc723624bbceacdcc75d4c7c45f5aec1d01
Author: Willem Wyndham <wyndham@cs.unc.edu>
Date:   Thu Nov 18 05:45:44 2021 -0500

    fix: add check to suppress retry log (#725)
    
    * fix: add check to suppress retry log

⋊> ~/P/N/t/near-api-js on gitHooks ↑ git diff 9b7108464db607179e2e9396ee56a8c6c6ec878e~ 9b7108464db607179e2e9396ee56a8c6c6ec878e                       15:53:01
diff --git a/lib/account.js b/lib/account.js
index ed5541e9..d3d74504 100644
Binary files a/lib/account.js and b/lib/account.js differ
diff --git a/src/account.ts b/src/account.ts
index 25f19608..8f364b21 100644
--- a/src/account.ts
+++ b/src/account.ts
@@ -215,6 +215,7 @@ export class Account {
      */
     protected signAndSendTransaction(receiverId: string, actions: Action[]): Promise<FinalExecutionOutcome>
     protected signAndSendTransaction(...args: any): Promise<FinalExecutionOutcome> {
+        console.log("hey");
         if(typeof args[0] === 'string') {
             return this.signAndSendTransactionV1(args[0], args[1]);
         } else {

As you can see, we have committed changes in 2 files.
Have you runned yarn prepare? Can you share your commands?

@ailisp
Copy link
Member

ailisp commented Jan 6, 2022

@volovyk-s sorry I didn't try. Try your commands and I can confirm the js files is generated and committed too - There's no need to commit again manually.
Try:

# do some changes on src/account.ts
git add -A
git commit -m "test"

changes to src/account.js is also commited

@itegulov I guess you might forget to git add or git add a wrong path? I did that once, git commit will still execute the hook, generate lib/account.js, but both files are not staged.

@itegulov
Copy link
Contributor

itegulov commented Jan 6, 2022

@volovyk-s @ailisp Ok, apparently it is being commited. What confused me was the commit message prompt I was using to judge which files are being staged:
latest-screenshot

Apparently it just lies to you and if you proceed with the commit everything is being commited as intended. Sorry for the confusion!

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.

Add a git hook to build the js files before commiting
4 participants