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: 🤖 run lint-staged on pre-commit hook #790

Closed
wants to merge 7 commits into from

Conversation

theashraf
Copy link

  • remove pretty-quick from pre-commit hooks and replace it with lint-staged
  • configure lint-staged linters for .js, .json, .md files
  • add format script to format the whole codebase (to be used when needed)

closes #788

@theashraf

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel self-assigned this Nov 14, 2019
.lintstagedrc.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@jorgeorpinel

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel jorgeorpinel removed their assignment Nov 14, 2019
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

@theashraf only a few things left to address in #790 (review) and #790 (review). Do you have time to finish this? I can do it if not, no worries. Thanks

@shcheklein shcheklein temporarily deployed to dvc-org-pr-790 November 15, 2019 02:05 Inactive
.lintstagedrc.json Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-org-pr-790 November 15, 2019 02:27 Inactive
.lintstagedrc.json Outdated Show resolved Hide resolved
.lintstagedrc.json Outdated Show resolved Hide resolved
@jorgeorpinel jorgeorpinel added the status: research Writing concrete steps for the issue label Nov 15, 2019
@shcheklein shcheklein temporarily deployed to dvc-org-pr-790 November 15, 2019 03:14 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Thanks a lot.

Just waiting to see if we can keep the --bail behavior pretty-quick had as discussed in #790 (comment).

Otherwise, we may have to settle for including a git add task (the developer would not realize that his code was automatically reformatted)... Would this be OK @shcheklein, @iAdramelk?

@iAdramelk
Copy link
Contributor

@jorgeorpinel I think that using git add here is ok. prettier shouldn't try to change anything that will break the code, and I don't see a lot of meaning for the developer to manually check prettier fixes, because we are forcing them anyway. So even if they didn't like the changes, it's not like that they can do something about it.

@jorgeorpinel
Copy link
Contributor

Thanks Alexey. Agree with you again. Let's just see if lint-staged provides a way to "bail", like pretty-quick did. (Waiting for lint-staged/lint-staged/issues/730.) Although, yet another option is simply to keep pretty-quick... I'l check with Ivan directly about this.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Changes are all good. Thanks again @theashraf!

But not ready to merge just yet. Waiting on an update. See my #790 (comment) above.

I'll keep an eye on this.

@jorgeorpinel

This comment has been minimized.

@jorgeorpinel
Copy link
Contributor

UDPATE: The lint-staged guys are actually implementing this 😬 See lint-staged/lint-staged/pull/745

@jorgeorpinel
Copy link
Contributor

So unfortunately the lint-staged guys ended up changing their minds about implementing this so let's keep pretty-quick --bail for now? I think it's important being able to know what you're committing without automatic git add. We can keep this PR alive (will get stale but oh well) to check for future versions of lint-staged though, as they hinted maybe they'll try to implement in a future version.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Dec 3, 2019

p.s. apologies to @theashraf who was great at submitting this PR. This is very rare that we don't end up merging them, especially when they come from an open issue (which I'm closing for now, BTW).

Abdelrahman, if you're interested in any other issue please feel free to check with me or anyone on the team (#dev-docs channel of our chat) before having a go just so we double check the issue validity. Again, this situation is very rare, in fact it's the first time it happens that I'm aware. But still, sorry!

@shcheklein
Copy link
Member

Closing this, @jorgeorpinel , right?

@shcheklein shcheklein closed this Dec 3, 2019
@jorgeorpinel
Copy link
Contributor

OK in that case I'll reopen the issue and relabel it. I guess that makes more sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: research Writing concrete steps for the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lint: switch pretty-quick for lint-staged in pre-commit hook?
4 participants