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: Update lint:text command for incoming document #454

Closed
wants to merge 7 commits into from

Conversation

no-yan
Copy link

@no-yan no-yan commented Dec 2, 2021

This PR is a part of #453.

概要

Textlintを実行するlint:textコマンドがbeta/src/pagesフォルダもlintするよう変更しました。
また、以下の変更を加えました。

  • globパターンをvalidに
  • cacheの有効化

globパターンをvalidに

lintされていないディレクトリが存在したため(content/home/{examples, marketing})、これを修正しました。

TextLintではglobパターンを引用符で囲わなかった場合、Textlintはサブディレクトリの探索を行いません。修正方法はtextlintのindex.mdに従いました。

When running texlint, you can target files to lint using the glob patterns. Make sure that you enclose any glob parameter you pass in quotes.

cacheの有効化

エラーを懸念してやらないことに決定しました。
lint対象が増えtextlintの処理時間が長くなることが予想されたので、--cacheオプションを有効化し高速化しました。手元の計測では12.8s => 2.3sと5.5倍高速です。

note:

MDX対応は行いませんでした。これは新ドキュメントの拡張子もすべて.mdで必要なかったためです。
textlint-plugin-markdownで.mdx拡張子をlintすることは可能ですが、これは.mdと同じ方法でlintしているだけです。また、textlintのmdxルールセットは見つかりませんでした。

@no-yan
Copy link
Author

no-yan commented Dec 2, 2021

動作の確認として以下の事項をテストし、問題がないことを確認しました。

  • 更新したlint:textbeta/src/pagesとそのサブディレクトリがlintされること、
  • エラーを出したファイルをlint対象外のフォルダに移すとエラーが検出されなくなること
  • 既存のwarningの数、メッセージが変更されないこと

@no-yan no-yan changed the title chore: Update lint:text command forincoming document chore: Update lint:text command for incoming document Dec 2, 2021
@smikitky
Copy link
Member

smikitky commented Dec 2, 2021

beta ディレクトリの中はリポジトリのトップレベルとは独立した package.json や yarn.lock を持つ別環境であり、beta ディレクトリの中と外の世界(現行サイト)が互いに干渉しないことになっているように見えます。変則的な monorepo と言いますか。現行サイトは Gatsby で新サイトは Next.js ですし。

将来的には beta の中身がそっくりそのままリポジトリのルートを置き換え、現行のリポジトリルートにある(beta 以外の)内容はどこかにアーカイブされます。逆に、現在のリポジトリのルートにあるコンテンツやスクリプトはあくまで現行のドキュメントサイトを担当しており、betaディレクトリの中身について一切関知してはいけません。

このPRでは、現行サイトを構築するためのスクリプトが beta の存在に依存してしまっていますし、将来 beta がメインサイトとして昇格する際にも問題が生じてしまいます。面倒でも、beta という別環境に、現行の textlint と同様のセットアップを再現する必要があると思います。textlint/jp-reactjs-org-lint.js もそのまま beta 内にコピーする感じですね。

@smikitky
Copy link
Member

smikitky commented Dec 2, 2021

言い方を変えると、クローンした後に beta ディレクトリだけ全く別の場所に動かしたとしても、現行サイトは beta なしで完璧にビルドできる必要がありますし、beta は beta で親フォルダにあったファイルへの依存なしに完璧にビルドできる必要があります。

@smikitky smikitky added beta workflow Working environment improvements including linting and CSS styles labels Dec 2, 2021
@no-yan
Copy link
Author

no-yan commented Dec 3, 2021

レビューありがとうございます!たしかにこのPRのままでは移行時に困りますね。

Betaディレクトリで独立して運用できるように、betaのpackage.jsonにtextlintとrule-presetを追加し、設定ファイルをコピーしようと思います。

Copy link
Member

@smikitky smikitky left a comment

Choose a reason for hiding this comment

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

普段は、lint-staged という NPM パッケージを利用して、コミット前のフックでコミット直前に当該ファイルのみに textlint を走らせています。それについては #126 をご覧ください。

#126 相当のものもここで含めてからマージできれば理想なのですが、いかがでしょうか。大変そうならそちらは別PRでやるのでも大丈夫です。

beta 側では既に husky が使われていますので、そちらの別途インストールは不要です。lint-staged を使って、既存の husky のセットアップを調整する形になります。

あと本家側でもコミット前に全部 prettier や eslint をかけるのではなく lint-staged を使ったらどうなの、と私がもちかけており(reactjs/react.dev#4143)、タイミングによりそちらとの干渉がありえることにも留意ください。

package.json Outdated Show resolved Hide resolved
beta/package.json Outdated Show resolved Hide resolved
Copy link
Member

@smikitky smikitky left a comment

Choose a reason for hiding this comment

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

1カ所だけ細かいですが修正お願いします。

(ちょっと自分の環境固有っぽい問題で動作確認がとれていないのでapproveは少しだけお待ちください)

package.json Outdated Show resolved Hide resolved
@smikitky
Copy link
Member

うちの環境でこの PR をチェックアウトし、cd beta; yarn し、試しに何か commit しようとすると、fatal: not a git repository: '.git' というエラーが出てコミットできないのですが、同様のエラーは出ていますか? クリーンインストールしてもダメでした。

以下の Issue をヒントに調べたところ、どうも GIT_DIR という環境変数が何らかの原因で ".git" という文字列に設定されているのが原因です。git-cli が beta/.git という存在しないディレクトリのみを見に行っており、「親ディレクトリを順にたどって .git フォルダを探しに行く」という git-cli の通常動作が阻害されています。無理やり pre-commit ファイル内で unset GIT_DIR で環境変数消してみたらとりあえず動作はしました。

typicode/husky#584

もうちょっと調べてみます…

@smikitky
Copy link
Member

https://stackoverflow.com/a/56026616/1209240

どうも pre-commit フック内で cd しようとすると面倒なよう。

どうせ本家側で lint-staged いれた時にまた考え直すことになるので、とりあえず日本版でうまく動くこと優先でいいかなと思います

@no-yan
Copy link
Author

no-yan commented Dec 17, 2021

うちの環境でこの PR をチェックアウトし、cd beta; yarn し、試しに何か commit しようとすると、fatal: not a git repository: '.git' というエラーが出てコミットできないのですが、同様のエラーは出ていますか?

自分の環境ではcommitできてます。
WSL2 on Windows 11, Ubuntu Ubuntu 20.04.3 LTS.

とりあえず動くようにすることで良いと思いますが、この修正を@smikitkyさんにお願いできますか?
エラーが再現できる方が担当していただけたら安全だと思います。

@smikitky
Copy link
Member

@no-yan すみません、平日はなかなか時間がとれていないのですが "Allow edits from maintainers" というのはチェックされていますでしょうか。一度 push を試そうとしたら no write access と言われたので。

https://docs.github.com/ja/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

また、そちらの環境での git version の結果はどうでしょうか。どうも GIT_DIR をセットしているのは git 自身らしいので、もしかしたら自分の git version が古いのかも、というあたりを原因として疑っています。時間がかかってしまいすみません。

@smikitky smikitky added the help wanted Extra attention is needed label Dec 22, 2021
@no-yan
Copy link
Author

no-yan commented Dec 22, 2021

ありがとうございます。あまり急ぐ理由もないでしょうし、のんびりやっていきましょう。React 18リリースのタイミングでbetaに移行はないでしょうから。

以下の二点ですが、おそらく問題ないと思います。

  • git version 2.33.1(最新版が2.34.1)
  • "Allow edits from maintainers"チェックあり

メインブランチからPRしており、これが疑わしい場合にはブランチを変更しますので言ってください。

@smikitky
Copy link
Member

smikitky commented Jan 19, 2022

懸念だった reactjs/react.dev#4139 これが upstream でマージされ、これで目に見える問題は一通り解決しそうなので、これをこのリポジトリにマージするときに改めて見直したいと思います。遅くなり申し訳ありません。

@no-yan
Copy link
Author

no-yan commented Aug 11, 2022

一からPRを作ったほうが早いと思われる場合は、どなたでも躊躇なくPRを出していただいて構いません。
問題のエラーを自分の環境で再現できないため修正に取り組んでいませんが、ブロッカーになるならクローズされるほうが望ましいと思っています。

@smikitky
Copy link
Member

@no-yan お気遣いありがとうございます。そろそろ翻訳進めていかないとなーと思って手を付けられていないというだけです。仰る通り、これをベースにするより現状の最新からやりなおした方が早いとなったらそうさせていただくかもしれませんが、その場合はよろしくお願いします。

@smikitky
Copy link
Member

ごめんなさい、リポジトリ構成がとんでもなく変わったので、textlint のセットアップも全体的にゼロからやりなおすことにしました。こちらは close させていただきます。

新しい PR は #580 です。

@smikitky smikitky closed this May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta help wanted Extra attention is needed workflow Working environment improvements including linting and CSS styles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants