Skip to content

chore: Add commit hooks for testing and linting #149

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Aug 10, 2021
Merged

chore: Add commit hooks for testing and linting #149

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
25f9d5c
chore: Add commit hooks for testing and linting
bahrmichael Jul 30, 2021
19728a9
chore: Exclude installed husky scripts
bahrmichael Jul 30, 2021
3147952
fix: update husky gitignores
bahrmichael Jul 30, 2021
40906ec
fix: update husky gitignores
bahrmichael Jul 30, 2021
3714ccb
fix: newline
bahrmichael Jul 30, 2021
9d8bc12
chore: update shrinkwrap
bahrmichael Jul 30, 2021
0243e20
fix: Make hooks executable
bahrmichael Jul 30, 2021
074e8eb
fix: rebuild shrinkwrap with npm 7
bahrmichael Jul 30, 2021
b08d0df
refactor: rename prepare to init-environment
bahrmichael Jul 30, 2021
a7cbb03
docs: Update dev setup to reference semantics list
bahrmichael Aug 2, 2021
5605966
docs: Add PR step that mentions linting/testing and commit hooks
bahrmichael Aug 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .husky/pre-commit
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

npm run lerna-lint
4 changes: 4 additions & 0 deletions .husky/pre-push
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

npm run test
16 changes: 8 additions & 8 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,16 +35,16 @@ To send us a pull request, please follow these steps:

1. Fork the repository.
2. Install dependencies: `npm install`
3. Create a new branch to focus on the specific change you are contributing e.g. `git checkout -b improv/logger-debug-sampling`
4. Run all tests, and code baseline checks: `npm run test`
5. Commit to your fork using clear commit messages.
6. Send us a pull request with a [conventional semantic title](https://github.com/awslabs/aws-lambda-powertools-typescript/pull/67), and answering any default questions in the pull request interface.
7. Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

GitHub provides additional document on [forking a repository](https://help.github.com/articles/fork-a-repo/) and
3. Prepare utilities like commit hooks: `npm run init-environment`
4. Create a new branch to focus on the specific change you are contributing e.g. `git checkout -b improv/logger-debug-sampling`
5. Run all tests, and code baseline checks: `npm run test`
6. Commit to your fork using clear commit messages.
7. Send us a pull request with a [conventional semantic title](https://github.com/awslabs/aws-lambda-powertools-typescript/pull/67), and answering any default questions in the pull request interface.
8. Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

GitHub provides an additional document on [forking a repository](https://help.github.com/articles/fork-a-repo/) and
[creating a pull request](https://help.github.com/articles/creating-a-pull-request/).


### Conventions

Category | Convention
Expand Down
43 changes: 35 additions & 8 deletions npm-shrinkwrap.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
"main": "lib/index.js",
"types": "lib/",
"scripts": {
"init-environment": "husky install",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add this to a postinstal script potentially, so it gets run automatically?

Copy link
Contributor Author

@bahrmichael bahrmichael Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Based on recommendation from husky I did not add the prepare script to the pre- or post-install steps. Package managers seem to have discouraged this recently. Unless we have a strong opinion, I'd stick with the recommendation from husky. What do you think?

If a contributor forgets this step, they should still be able to contribute. They'll see failures in the PR builds if the violate the linting or tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always a fan of a few manual steps as possible, not a massive problem, but it's an extra step people need to read/act on

Copy link
Contributor

Choose a reason for hiding this comment

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

The recommendation from package managers appears sensible on the grounds of better security.

I think that if we do a good job of documenting the setup of the dev environment on the Contributing.md document, and remind the user to lint & test both in that document and in the PR checklist it should be enough.

Copy link
Contributor Author

@bahrmichael bahrmichael Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

in the PR checklist

Our PR builds already lint and test the contributor's code. We can add a checklist step indicating that they should lint&test, but the checklist step is not required to make them aware of failures. I'm not inclined to add a checklist step, as this would repeat work that our PR checks already do. I'm open to adding it, if there's another reason :)

Copy link
Contributor

@dreamorosi dreamorosi Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

True. I don't have a good answer though, I think we are all in agreement that the main branch is already protected against untested/un-linted code.

On the other hand we want to favor automation as much as possible, so the happy path here would be that a contributor installs and runs the hooks which in turn would mean these steps of the workflow would pass.

There's a non-zero chance that some contributors will forget to install (and perhaps run) these hooks and in case their code is not linted the workflow will fail. Its not at all a big deal since the contributor will see the error, fix it and commit again.

My reasoning was that having an additional check on the list that says "lint your code (this is done automatically via husky [link to relevant script/explanation])" could help avoid some frustration for some, especially those who are new to all this kind of tooling.

Either way all this is a supposition on my part, am more than happy to not add the item and reconsider if this ever becomes a problem :)

Copy link
Contributor Author

@bahrmichael bahrmichael Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

My reasoning was that having an additional check on the list that says "lint your code (this is done automatically via husky [link to relevant script/explanation])" could help avoid some frustration for some, especially those who are new to all this kind of tooling.

I understand now, great idea! I'll work that in :)

"test": "npm run lerna-test",
"commit": "commit",
"lerna-ci": "lerna exec -- npm ci",
"lerna-test": "lerna exec -- npm run test",
Expand Down Expand Up @@ -39,6 +41,7 @@
"@typescript-eslint/eslint-plugin": "^4.11.1",
"@typescript-eslint/parser": "^4.11.1",
"eslint": "^7.16.0",
"husky": "^7.0.1",
"jest": "^27.0.4",
"lerna": "^4.0.0",
"ts-jest": "^27.0.3",
Expand Down