Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @sevenc-nanashi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a new formatter specifically designed for HTML content within Markdown files, enhancing the formatting capabilities of the repository. The changes include adding a script to format Vue-like segments in Markdown files, updating the linting and formatting commands in package.json to include this new script, and adjusting the .prettierignore file to exclude Markdown files from standard Prettier formatting. The goal is to automate and standardize the formatting of HTML and Vue code snippets embedded within Markdown documentation.
Highlights
- HTML Formatter: Adds a new formatter (
tools/format.ts) to handle HTML and Vue-like code segments within Markdown files. This formatter usesprettierandmdast-util-from-markdownto parse and format these segments. - Linting and Formatting Commands: Updates the
lintandfmtscripts inpackage.jsonto include the new HTML formatter, ensuring that Markdown files are properly formatted during the linting and formatting processes. - .prettierignore Update: Modifies the
.prettierignorefile to exclude Markdown files from the standard Prettier formatting, as these files will now be formatted using the custom HTML formatter. - README.md Update: Adds a section to the README explaining how to run the formatter.
Changelog
Click here to see the changelog
- .prettierignore
- Added an entry to exclude Markdown files from standard Prettier formatting.
- README.md
- Added a 'フォーマット' section explaining how to run the lint and format commands.
- package.json
- Updated the
lintscript to include the new HTML formatter. - Updated the
fmtscript to include the new HTML formatter. - Added
@types/diff,diff, andmdast-util-from-markdownas dev dependencies.
- Updated the
- pnpm-lock.yaml
- Updated dependencies to reflect changes in
package.json.
- Updated dependencies to reflect changes in
- src/architecture.md
- Minor formatting changes to image tags.
- tools/format.ts
- Added a new script to format HTML and Vue-like code segments within Markdown files.
- The script uses
prettierfor formatting andmdast-util-from-markdownto parse Markdown files. - The script supports
--checkand--writeflags for checking and applying formatting changes, respectively. - The script uses
diffto display verbose output.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the purpose of the 'mdast-util-from-markdown' library used in the formatting script?
Click here for the answer
The 'mdast-util-from-markdown' library is used to parse Markdown content into an abstract syntax tree (AST), allowing for the extraction and manipulation of specific elements, such as HTML and Vue-like code segments, within the Markdown structure.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new HTML formatter, enhancing the project's ability to handle HTML content within Markdown files. The changes include adding necessary dependencies, updating scripts, and implementing the formatting logic. Overall, the implementation seems well-structured and addresses the intended functionality.
Merge Readiness
The pull request appears to be in good shape for merging. The changes are well-organized and address the intended functionality of adding HTML formatting support. I am unable to directly approve the pull request, and that users should have others review and approve this code before merging.
| if (check && count > 0) { | ||
| throw new Error("Some files need formatting"); | ||
| } |
There was a problem hiding this comment.
（ただのコメントです）
これくらい小さいのを関数化するべきなのかがわからないんですよね～～～
Hiroshiba
left a comment
Hiroshiba
left a comment
There was a problem hiding this comment.
結構読みやすくなった気がします！！
あと早期 continue と、できればforの中身切り出しも。。
あと関数の並び順がぐちゃぐちゃになっちゃってるかもです 🙇
（あんまコード整理の恩恵感じないかもですが、慣れてくればきっと有用に感じると思うので。。。きっと。。。）
| let changedCount = 0; | ||
| for await (const filePath of glob("./src/**/*.md")) { | ||
| console.log(`${filePath}:`); | ||
| const source = await readFile(filePath, "utf8"); | ||
|
|
||
| const result = await formatMarkdown(source); | ||
| if (source !== result) { | ||
| if (mode === "check") { | ||
| console.log(styleText("red", " Needs formatting")); | ||
| } else { | ||
| await writeFile(filePath, result); | ||
| console.log(styleText("green", " Formatted")); | ||
| } | ||
|
|
||
| if (verbose) { | ||
| printDiff(filePath, source, result); | ||
| } | ||
| changedCount++; | ||
| } else { | ||
| console.log(styleText("gray", " No changes")); | ||
| } | ||
| } |
There was a problem hiding this comment.
forの中身を
入力 filePath, mode, verbose
出力 changed
な関数に切り出せば
let changedCount = 0;
for await (const filePath of glob("./src/**/*.md")) {
const changed = await formatOneFile({ filePath, mode, verbose });
if (changed) changedCount++;
}というわかりやすいコードになるはず。
更に↑の５行のコードを更に関数に切り出せば
const changedCount = formatFiles(glob("./src/**/*.md"), mode, verbose)というめちゃくちゃ直感的な関数になるはず。
（そしてこうなってくるとmodeやverboseをグローバル変数にしたくなってくる）
| if (mode === "check") { | ||
| console.log(`${changedCount} files need formatting`); | ||
| if (changedCount > 0) { | ||
| throw new Error("Some files need formatting"); | ||
| } | ||
| } else { | ||
| console.log(`Formatted ${changedCount} files`); | ||
| } |
There was a problem hiding this comment.
（ただのコメントです）
慣れてくるとこれですら複雑なコードに見えるんですよね。。。
if (mode === "check") {
console.log(`${changedCount} files need formatting`);
} else {
console.log(`Formatted ${changedCount} files`);
}
if (mode == "check" && changedCount > 0) {
throw new Error("Some files need formatting");
}と分解したくなる･･･
そして少なくとも前者は関数に切り出したくなる。。。
There was a problem hiding this comment.
これ、目的の違う処理が同じブロックに書かれてるので、たぶんロジック分けた方がわかりやすいですね！！
仮に関数に切り出した時、すんごい長い名前の関数になっちゃうと思います。
「printChangedCountAmdThrowWhenShouldWrite」みたいな。
ということは処理がまとまってないことがわかり、処理を崩すようになる。
関数切り出ししないにしても、切り出せるようにロジックは混ざらないように気をつけると良いのかも（気づき）
|
出力部分を外に出しました。 |
| for await (const filePath of glob("./src/**/*.md")) { | ||
| console.log(`${filePath}:`); | ||
| const source = await readFile(filePath, "utf8"); | ||
| process.stdout.write(`${filePath}: `); |
There was a problem hiding this comment.
お。ここconsole.logを使わない理由とかありますか👀
There was a problem hiding this comment.
process.stdout.writeは改行なしの出力ですね。ちょっとコンパクトになります。
There was a problem hiding this comment.
あーーーなるほどです！！2つ課題がありそう。
まずこの次のログが、このログより右にあってもおかしくないものを想定していて、ロジックが閉じてなさそう。
たぶんprint関数にしてprintResultの近くに配置するとまとまりそう。
（まあでもあまり名無し。さんの好きな考え方ではないかも）
あと意図がわかりづらそう。
これは専用の名前を付けた別関数を定義すれば解決だけど、まあもっとスクリプトが増えた時に共通化すれば良さそう。
まあ、そういう課題があるよという共有ということで！
There was a problem hiding this comment.
エラー吐いた時のためにフォーマット前に出力しておきたいんですよね。
今がこんな感じで：
hoge: No Changes
fuga:（エラー）
print側にまとめるとこうなるはず：
hoge: No Changes
（エラー）
There was a problem hiding this comment.
あっすみません！！そうだと思います！
処理が関数を超えるのは仕方ないので、せめてprintStartを切り出してprintResultを近場に置いとくと少しわかりやすいかなぁ、という意図でした！！
まあそもそもログをこんな丁寧に書く必要は（このリポジトリにとっては）あまりなく、わりとオーバースペックかもですね。
（オーバースペックなことに今気づきました）
| }); | ||
|
|
||
| async function main() { | ||
| console.log( |
There was a problem hiding this comment.
このログはparseArgs側のが良いかも。（この関数がしたい処理ではないので）
あとパッとわからんコードになってるので崩す癖をつけてくと良いかも…？
だけどまあここだけなら別に良さそう！
他にも現れたら共通化すれば良さそう。
There was a problem hiding this comment.
改行位置を変えました。ちょっとはマシになったはず...？
There was a problem hiding this comment.
そもそもparseArgsがパースした結果だから、parseArgs側に置いたほうが場所合ってる気が。
There was a problem hiding this comment.
parseがprintするのはなんかなぁって気持ちがあります（parseAndPrintArgs...？）
There was a problem hiding this comment.
まあそもそも
mode === "check" ? styleText("green", "Check") : styleText("red", "Write")が結構分かりづらいんですよね。
styleText("green", "Check")はただ文字列が２つ並んでるだけで、何がやりたいのかぱっとわからない。
たぶんgreen("Check")と呼べるように関数化したほうがわかりやすそう。
console.log(
`Mode: ${
mode === "check" ? green("Check") : red("Write")
}${verbose ? " (Verbose)" : ""}`,
);まあこれ以上は制御変数できちゃうのでここ止まりかなぁ。
There was a problem hiding this comment.
となるとchalkとか入れた方がよさそう？（chalk.green("Check")みたいなAPIのはず）
There was a problem hiding this comment.
parseがprintするのはなんかなぁって気持ちがあります（parseAndPrintArgs...？）
今実際printしてますね！-hとかで。
ちょっとログをどう扱うのが良いかはまだわかってないんですよね～～～～～～
他のグローバルな処理、例えばエラーを投げるときは@throws書けばいいけど、ログとかあとダイアログ表示とかをどう考えれば良いかわからない。。。。
手探りで探していければ 😇
There was a problem hiding this comment.
となるとchalkとか入れた方がよさそう？（chalk.green("Check")みたいなAPIのはず）
将来的に入れても良いかもだけど、今はまだ時期尚早な気がする、という感じです！
リポジトリの成長次第では導入しても良いかもだけど、今のとこ仮にスクリプト増えてもhelper関数作るだけで十分な気がする。
2 participants
内容
HTMLに対応したフォーマッターを追加します。
関連 Issue
（なし）
スクリーンショット・動画など
（なし）
その他
（なし）