-
Notifications
You must be signed in to change notification settings - Fork 309
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
generateLicenses.jsを書き直す #2000
generateLicenses.jsを書き直す #2000
Conversation
const fs = require("fs"); | ||
const fs = require("fs/promises"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
別に良いけど実は不要な変更かも。
nodejsはpromiseの方が推奨されてるとかあったりするなら変更したほうが良さそう。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
メインがasyncになったのでせっかくなので、って感じです。どちらでも良さそう。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
なるほどです、まあ良さそう。
仮に戻すってなった時のこと考えると、どちらでも良いならたぶん変えない方が良さそう。
けどまあpromiseは取り回しやすいし(await忘れが怖いけど)、関数内だからどちらにせよ差分は発生するし、まあ良いかな。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ほぼLGTMです!!
細かい点をいくつかコメントしました!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
ちなみにlicense-checker-rseidelsohnに変えた理由はdeprecated警告をなくすためですよね。
ざっとリポジトリ眺めた感じ、良さそう。
* Change: generateLicenses.jsを書き直し * Fix: execFileSync周りの型を修正 * Code: argvを離す * Fix: 細かいところを修正
内容
関連 Issue
スクリーンショット・動画など
(なし)
その他
キリ番踏んだので報告します!()
build下のスクリプト、そのうちTypeScriptに書き換えて安心したい