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.

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

Jump to bottom

[DNM] TFAC-292 Lint on Pre-Commit #153

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[DNM] TFAC-292 Lint on Pre-Commit #153

wants to merge 1 commit into from

Conversation

jedtan
Copy link
Contributor

@jedtan jedtan commented Mar 30, 2021

No description provided.

@jedtan jedtan requested a review from kmjennison March 30, 2021 06:32
@vercel
Copy link

vercel bot commented Mar 30, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/gladly-team/tab-web/Hig9tH1MzT1oZejfccwYNSMJbM5v
✅ Preview: https://tab-web-git-jtan-husky-pre-commit-gladly-team.vercel.app

[Deployment for 1bfb75e failed]

@codecov
Copy link

codecov bot commented Mar 30, 2021 •
edited
Loading

Codecov Report

Merging #153 (777e5b2) into master (9880c7d) will not change coverage.
The diff coverage is n/a.

❗ Current head 777e5b2 differs from pull request most recent head 1bfb75e. Consider uploading reports for the commit 1bfb75e to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #153   +/-   ##
=======================================
  Coverage   90.22%   90.22%           
=======================================
  Files          86       86           
  Lines        1217     1217           
  Branches      289      289           
=======================================
  Hits         1098     1098           
  Misses        107      107           
  Partials       12       12

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 9880c7d...1bfb75e. Read the comment docs.

amaliwanag2
amaliwanag2 reviewed Mar 30, 2021
View reviewed changes
package.json
@@ -7,14 +7,15 @@
"dev": "echo \"Use the 'yarn go' command to develop locally.\"",
"build": "NODE_ENV=test yarn run test && NODE_ENV=production yarn run relay && next build",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want to add yarn run lint to this build command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds good.

kmjennison
kmjennison requested changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@kmjennison kmjennison left a comment

Choose a reason for hiding this comment

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

Definitely add lint checking in the build command. My other suggestions are open-ended: up to you if you want to add them now or not.

package.json
@@ -7,14 +7,15 @@
"dev": "echo \"Use the 'yarn go' command to develop locally.\"",
"build": "NODE_ENV=test yarn run test && NODE_ENV=production yarn run relay && next build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds good.

.husky/pre-commit Outdated
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

yarn run lint && yarn test
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be useful to have Husky fix linting errors when possible and commit the fixes (e.g., this article on using lint-staged). What do you think?

I also saw a suggestion to run only affected tests, like using Jest's --changedSince=master or --onlyChanged flags, which sounds great. I'm a little skeptical of the overhead of running the whole test suite on commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a great idea - I'm currently struggling getting any flags (--changedSince, --onlyChanged and --findRelatedTests) to pick up, well, any tests, which is slowing down my velocity on this somewhat.

@kmjennison kmjennison changed the title TFAC-292 Lint on Pre-Commit [DNM] TFAC-292 Lint on Pre-Commit Oct 20, 2021
@kmjennison kmjennison marked this pull request as draft September 4, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaliwanag2 amaliwanag2 amaliwanag2 left review comments

@kmjennison kmjennison kmjennison requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jedtan @kmjennison @amaliwanag2