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: Add Husky/lint-staged for local precommit #20397

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Sep 7, 2022

Will run Prettier/Markdownlint if the contributor is running locally.
It may be too early to run the Prettier hook, as it will cause many changes in some files if it's being formatted for the first time

@caugner caugner requested review from dipikabh, Rumyra and a team September 7, 2022 18:28
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, but pinged some folks to make sure this has consensus.

@caugner caugner requested a review from bsmth September 7, 2022 18:29
@@ -0,0 +1,4 @@
{
"*": "prettier --ignore-unknown --write",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this should be disabled till a pass is made to the repo to baseline the amount of changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, disabling for now definitely makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other option would be to keep this, but use .prettierignore till PRs like #19943 clean/baseline up particular folders

@Josh-Cena
Copy link
Member

We're not ready for wide adoption of Prettier yet. It would require using js-nolint, and also use prettier-ignore in a lot of places where the formatting is intentional.

I have less opinions on running Markdownlint.

@nschonni
Copy link
Contributor Author

nschonni commented Sep 7, 2022

Remembered I had a separate take on this before Markdownlint https://github.com/mdn/content/pull/394/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R11
I won't add it here, but something to consider if the flaws could also be cleaned up safely with a hook

@teoli2003
Copy link
Contributor

teoli2003 commented Sep 8, 2022

We should comment out Prettier: if we are not ready to have a workflow adding warnings, we are not ready for an auto-fix.

For the Markdownlint part, we should speak about it with the usual reviewers/committers to reach a consensus first. (Like in our editorial meetings on Mondays).

It would be the first use of Husky on mdn/content, which would be quite surprising, so I think it is worth presenting this before it appears on their local yari. They are also the people who will need to help other editors in case of problems.

So I think we should follow this course of action here:

  1. Comment out Prettier tasks
  2. Present it in the editorial meeting to check if there is a consensus. (If there is one, continue with 3.)
  3. Activate it
  4. Announce it on the discussion board. Should people just update with yarn install, or should they install something locally, and how (like markdonwlint)?

cc/ @Rumyra @schalkneethling

Working by consensus is hard (and sometimes frustrating) but leads to much better and solid solutions.

@caugner
Copy link
Contributor

caugner commented Sep 8, 2022

4. Should people just update with yari install,

After running yarn or yarn install the git pre-commit hook would automatically be installed.

PS: We have been using lint-staged + husky successfully in mdn/yari, avoiding failing lint checks, and I have used it in other projects for at least 3 years before that.

@nschonni
Copy link
Contributor Author

nschonni commented Sep 8, 2022

Updated the .prettierignore to skip all markdown except for some folders I cleaned up in some other PRs. There was a minor one with a JS file I included in that it picked up running pretter -w . at the root

.prettierignore Outdated
Comment on lines 7 to 8
!files/en-us/games/**/*.md
!files/en-us/related/**/*.md
Copy link
Contributor

@teoli2003 teoli2003 Sep 9, 2022

Choose a reason for hiding this comment

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

So, this means that husky Prettier will be enabled on games/ and related/?

I think we should keep our PR simple. Adding Husky/lint-staged here for Markdownlint and activating Prettier for these directories in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I'm fine to have this file added here, except for line 7-8, which should be in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those folders were previously clean up in #19949 and #19943

Copy link
Contributor

@teoli2003 teoli2003 Sep 9, 2022

Choose a reason for hiding this comment

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

Yes, I know.

We are getting there, but I feel people have concerns, that is, fear that their workflow will get broken and lower their productivity. Breaking the move into independent smaller steps helps mitigate these. For example, we found a small bug in the pr-markdownlint-workflow today, but thanks to the step-by-step approach, it was detected early, and no other editors noticed it.

Running Husky/lint-staged for Markdown is the next logical step: authors that have noticed the two Markdownlint workflows and worked with them are now – I think – confident they work well, so will be happy to have the pre-commit hook set up (I hope).

Prettier is another beast:

  • There is a general agreement we want to use it (Yeah! 🎉 )
  • There is no agreement on the config file (Hence the request to have this in a separate PR)
  • Unlike Markdownlint, we don't have the two workflows set up, so people are not confident about it working well.

I don't want to hold this PR until we have agreed on the config file, set up the two workflows, and demonstrated their innocuity. (We can set up the workflows as soon as we have a consensus on the configuration, especially as we can activate them only in some areas)

I aim to have authors/reviewers happy with these automatic tools, hence the step-by-step approach.

Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

Activation of prettier for 2 directories should be in its own separate PR.

@teoli2003
Copy link
Contributor

I'm happy that this pre-hook commit will prevent many Prettier problems with our JSON files like GroupData.json. This will save time while keeping these files tidy.

.prettierignore Outdated Show resolved Hide resolved
@teoli2003 teoli2003 requested a review from a team as a code owner September 13, 2022 10:15
@teoli2003
Copy link
Contributor

So we have a consensus to run Husky (double-checked in the Monday editorial meeting), but not for Prettier on MD files. Using Husky for Prettier on MD files is something we will do in the future, but we are just not ready yet.

I've updated the ignore file to reflect this.

I'm merging this.

@teoli2003 teoli2003 merged commit 63015b4 into mdn:main Sep 13, 2022
@nschonni nschonni deleted the lint-staged-husky branch September 13, 2022 13:21
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* chore: Add Husky/lint-staged for local precommit

* chore: ignore non-cleaned markdown folders

* Do not run husky with Prettier on any MD file (for the moment)

Co-authored-by: Jean-Yves Perrier <[email protected]>
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