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 the cspell in husky #724

Closed
wants to merge 5 commits into from
Closed

add the cspell in husky #724

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 8, 2023

Resolves #562

@ubiquibot
Copy link

ubiquibot bot commented Jul 8, 2023

@ghost
Copy link
Author

ghost commented Jul 8, 2023

@pavlovcik can u take a look

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

That doesn't seem right because the purpose of lint-staged is to run logic on staged files. Why cspell the entire codebase when only the changed files need to be cspell'd

You should debug lint-staged

.husky/pre-commit Outdated Show resolved Hide resolved
},
"husky": {
"hooks": {
"pre-commit": "yarn lint-staged && yarn cspell $(find . -type f \\( -name '*.sol' -o -name '*.tsx' -o -name '*.ts' \\) ! -path '*/node_modules/*' ! -path '*/lib/*' ! -path '*/types/*') --no-must-find-files",
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure you're running the logic redundantly here, which makes the commit significantly slower to complete.

@ghost
Copy link
Author

ghost commented Jul 9, 2023

That doesn't seem right because the purpose of lint-staged is to run logic on staged files. Why cspell the entire codebase when only the changed files need to be cspell'd

You should debug lint-staged

Like cspell is actually running on just the staged files I checked 🤔 may be try running it locally it should be working

@0x4007
Copy link
Member

0x4007 commented Jul 9, 2023

Unfortunately I need some time to review this because I wrote the bounty five months ago and don't exactly recall what I'm referring to.

I presume people were pushing changes before without cspell.

Now that I think about it, it's possible that the husky hook wasn't being modified after old contributors already had the project forked and cloned.

@molecula451 @rndquu would you guys be able to do a quick test to confirm this hypothesis?

So to clarify, check if the husky hook gets updated if you already cloned a repository, and yarn installed the dependencies. I believe that it's possible the hook was modified to support cspell but the husky precommit hook wasn't being updated (with cspell) on existing contributor local copies.

If that's correct then that means the bounty was not necessary in the first place.

I forgot to spend some days to review and validate all these old bounties.

@molecula451
Copy link
Member

molecula451 commented Jul 9, 2023

as @pavlovcik says judging by the input i think this was better for a discussion and feedback, before opening a PR topic on the same issue #562, there was even a closed PR where was recalling the changes "unnecesary" on some reviews #717. That being said I'm taking pavlovcik words and giving a spin-off to this PR to really tackle in what is actually solving and if the issue still exists & persists on the repo after months

@0x4007
Copy link
Member

0x4007 commented Jul 9, 2023

It's possible that it's a non issue though with the new wave of contributors and the new husky lint-staged hook already including cspell. It would be great if you can verify that.

@molecula451 molecula451 self-requested a review July 9, 2023 09:38
@ghost ghost requested a review from 0x4007 July 9, 2023 12:23
@molecula451
Copy link
Member

This PR is potentially introducing unwanted complexity, while @AnakinSkywalkeer it's doing it's best to meet requirements, the output it's not really clear, as the complexity will mirror the Spell Check CI, we are already relying on Spell Check.yml for linting checks, so this husky pre-commit would work out locally, but at the same time adding redundancy to the project. Like right because we already have spell-check.yml and linting check when pushing changes. I have edited the original requirements that pavlovcik wrote as it looks more reasonable from that stand point of view. Beyond that. I would close this PR and issue as not planned, and only re-opened for further discussion when need it. Giving the oportunity to the hunter on a possible future if the issue is re-opened so is we decide to give updates and maintenance to this husky package

@molecula451 molecula451 closed this Jul 9, 2023
@0x4007
Copy link
Member

0x4007 commented Jul 11, 2023

https://chat.openai.com/share/084a9c22-efea-4ad8-93e3-f2c67ca6e66e

  1. Commit the changes and push to the repository.
  2. Ask your contributors to pull the latest changes.
  3. Have them run npx husky install (or yarn husky install) to update the Git hooks on their local repository.

@molecula451
Copy link
Member

molecula451 commented Jul 11, 2023

https://chat.openai.com/share/084a9c22-efea-4ad8-93e3-f2c67ca6e66e

  1. Commit the changes and push to the repository.
  2. Ask your contributors to pull the latest changes.
  3. Have them run npx husky install (or yarn husky install) to update the Git hooks on their local repository.

Looks redudant by the GPT, this is done by yarning at first sight, because husky is in package.json @pavlovcik

@0x4007
Copy link
Member

0x4007 commented Jul 11, 2023

Step 3. says otherwise.

I think that old contributors need to run npx husky install in order to update the commit hooks.

@molecula451
Copy link
Member

Step 3. says otherwise.

I think that old contributors need to run npx husky install in order to update the commit hooks.

We'll surely come out with something for this one great pavlovick!

@rndquu
Copy link
Member

rndquu commented Jul 11, 2023

We already use husky install in the postinstall script but the postinstall script sometimes is not executed on yarn install. There is the long running issue for this case.

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.

cspell ci
3 participants