search

codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
28 stars 25 forks source link

373 husky precommit prettier linter #385

Closed eathren closed 11 months ago

eathren commented 1 year ago

name: Pull Request about: Create a pull request to merge your code into the PASS codebase title: 'Add Husky precommit hooks to run prettier, linter, and vitest on commit for checks before they reach CI. ' labels: 'husky prettier pre-commit eslint' assignees: @nolanbraman

CONCISE description of PR (PR Title)

This runs tests, prettier, and linter before git commit completes

This PR:

1.
2. If needed, delete if not
3. If needed, delete if not

The files this PR effects:

Components

All .js, .jsx, .ts, .tsx files and tests

Tests

This runs all tests on every commit.

Other Files

List file names here

Screenshots (if applicable):

Add any screenshots/videos here.

Additional Context (optional):

Add any other context about the PR here.

Future Steps/PRs Needed to Finish This Work (optional):

Add any other steps/PRs that may be needed to continue this work if this PR is just a step in the right direction.

Issues needing discussion/feedback (optional):

1.
2. If needed, delete if not
3. If needed, delete if not

leekahung commented 1 year ago

Hey Nolan, with regards to this PR, I'm not too familiar with husky. Was wondering if there would be some pointers in the description as to how this PR is intended to be used with the linting, formatting and testing. Thanks!

eathren commented 1 year ago

@leekahung, that is a good point :^)

running npm run prepare to install the husky pre-commit hook. I'll go ahead and add a section to the README under installation to help others with this as well, since it is definitely an opt-in feature.

andycwilliams commented 1 year ago

Just checking to see if there are any updates with this PR.

bingeboy commented 11 months ago

+1 for adding husky

aedwardg commented 11 months ago

I'd recommend not running the tests in the pre-commit hook. 1. that would unblock this PR, and 2. it's less likely to slow down development significantly. Requiring all tests to pass before committing can be a roadblock, especially to newer developers.

bingeboy commented 11 months ago

IMHO it'd be nice to have testing, linting, and prettier done prior to commit. Having new devs skip testing doesn't sound like an ideal situation since we have limited testing resources and they can always make husky skip testing when pushing to remote for peer review. I've worked both ways in the past... I mainly think have linting and prettier run pre-commit is a nice workflow.

timbot1789 commented 11 months ago

I think I agree with @aedwardg here. We can add the unit testing hook later if it's useful. Until then, we can remove it and get this unblocked.

timbot1789 commented 11 months ago

I'll take over and finish up this PR

timbot1789 commented 11 months ago

I removed the test call from this. Our tests run oddly slow right now (~40 seconds, but they should run in ~4 seconds), and I don't think we need to run that on every commit. If someone likes committing work frequently, that delay could get annoying.

We can consider adding it back in once we figure out what's causing tests to run so slowly.

timbot1789 commented 11 months ago

@leekahung can you review again?

leekahung commented 11 months ago

@leekahung can you review again?

I don't have my computer with me on my trip, so I couldn't really check if the fixes are good on my end. But if you guys think this is ready, I'm good with approving this.

timbot1789 commented 11 months ago

@leekahung can you review again?

I don't have my computer with me on my trip, so I couldn't really check if the fixes are good on my end. But if you guys think this is ready, I'm good with approving this.

Ah ok. In that case I'll just dismiss your change request and merge it.