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

Update Husky and linting configuration #328

Merged
merged 3 commits into from
Jul 27, 2021
Merged

Update Husky and linting configuration #328

merged 3 commits into from
Jul 27, 2021

Conversation

keeganwitt
Copy link
Collaborator

@keeganwitt keeganwitt commented Jun 23, 2021

What

Update Husky and fix linting configuration.

Why

Linting is not being performed currently by the Git hook.

@mattphillips
Copy link
Member

I'm not sure I agree with removing the pre-commit hook. Typically I am also against them when they are blocking but in this case all it does is run prettier on the staged changes and is non-blocking.

I agree that it would be good to add a prettier based check to CI also to check consistency as pre-commit hooks are skippable something like: https://prettier.io/docs/en/cli.html#--check

@rickhanlonii
Copy link
Contributor

Non-blocking is ok I guess - we should definitely add a check to CI though.

@keeganwitt keeganwitt changed the title Remove Husky Update Husky and linting configuration Jun 23, 2021
@keeganwitt
Copy link
Collaborator Author

keeganwitt commented Jun 23, 2021

Updated to update Husky and the linting configuration so that it runs, rather than removing Husky. Let me know if this isn't what you had in mind.

@keeganwitt keeganwitt mentioned this pull request Jun 23, 2021
package.json Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 10, 2021

Codecov Report

Merging #328 (00e6e85) into main (a0d7d65) will not change coverage.
The diff coverage is n/a.

❗ Current head 00e6e85 differs from pull request most recent head 9587ebf. Consider uploading reports for the commit 9587ebf to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main      #328   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          109       109           
  Lines          636       636           
  Branches       100       100           
=========================================
  Hits           636       636           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0d7d65...9587ebf. Read the comment docs.

.prettierrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
*.js text eol=lf
Copy link
Collaborator Author

@keeganwitt keeganwitt Jul 10, 2021

Choose a reason for hiding this comment

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

This probably isn't the best way to fix this, but I tried adding

"endOfLine": "auto",

to prettier in package.json. but I was still getting lint warnings (on Windows with core.autocrlf true).

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to figure out how to not do this, no project I've used with prettier needs it so I'm skeptical that this is the best solution.

@keeganwitt keeganwitt requested a review from SimenB July 10, 2021 23:20
Copy link
Contributor

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

@rickhanlonii rickhanlonii merged commit 7c2bf3a into jest-community:main Jul 27, 2021
@keeganwitt keeganwitt deleted the upstream/keeganwitt/remove_husky branch July 28, 2021 00:23
@SimenB
Copy link
Member

SimenB commented Oct 6, 2021

note that this didn't actually run - had to chmod it: 44b9e6c

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.

4 participants