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

chore: clean-up code base #1230

Closed
wants to merge 2 commits into from
Closed

chore: clean-up code base #1230

wants to merge 2 commits into from

Conversation

paescuj
Copy link
Contributor

@paescuj paescuj commented Jan 5, 2023

First of all, thank you very much for providing and maintaining husky ❤️

While working on another pull request I realized that there are currently some lint issues (popping up in VS Code). So I decided to fix them and take the chance to give the whole code base a little clean-up:

  • Linting
    • Fix indentation errors from ESLint
    • Add lint step to pre-commit and GitHub workflow
  • GitHub workflows
    • Use more appropriate naming for jobs
    • Consolidate test jobs into one by using the matrix exclude option for Windows with Node.js 14
  • husky.sh shell script
  • TypeScript files
    • Convert comments on functions and variables to JSDoc, so we profit from the descriptions in IDEs
    • Add / enhance some comments
  • test shell scripts
    • Apply recommendations from shellcheck
    • Unset GIT_INDEX_FILE env var when called via git hook, that is running git commit in this very repo.
      Otherwise git commands in tests are running into conflicts because they are using the index from this repo instead of the temporary test repos. Honestly, I'm a bit surprised this hasn't been an issue for others before...
    • Use printf instead of echo -e for true POSIX (for example, special chars were printed instead of colored output on macOS)
  • tsconfig.js file
    • Expand from node14 config since we're targeting Node.js >=14
  • CONTRIBUTING.md file
    • Fix reference to LICENSE file

@typicode
Copy link
Owner

typicode commented Mar 7, 2023

Thank you, I'll review it with other PRs for the next major release (planned for the end of life of Node 14).

@typicode
Copy link
Owner

Thanks again for the PR 👍 I may not merge everything so I'm splitting it into smaller PRs and adding you as co-author for attribution.

@paescuj
Copy link
Contributor Author

paescuj commented May 2, 2023

Thanks again for the PR 👍 I may not merge everything so I'm splitting it into smaller PRs and adding you as co-author for attribution.

What's wrong with the other changes?
Should we close this PR now?

@typicode
Copy link
Owner

typicode commented May 5, 2023

Nothing wrong, mostly personal taste (for example, not sure if I want to add shellcheck style and I prefer // style for comments in JS).
Also changelog will maybe be generated, with multiple PRs it's clearer/more readable.

I'm keeping this PR open for reference, I'll close it later :)

@paescuj
Copy link
Contributor Author

paescuj commented May 5, 2023

I see, thanks for letting me know! 👍

@typicode typicode closed this Sep 22, 2023
@paescuj paescuj deleted the chore/clean-up-code-base branch September 22, 2023 04:55
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.

None yet

2 participants