-
Notifications
You must be signed in to change notification settings - Fork 311
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
fix: config.json の validation 失敗時にわかりやすいログを出すように #1222
Conversation
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.
PRありがとうございます!!
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!!
ありがとうございます、結構いろんな方が詰まるポイントだったので助かります!!
一点だけコメントしてみました!
src/background.ts
Outdated
log.error( | ||
`設定ファイルの読み込みに失敗しました。${app.getPath( | ||
"userData" | ||
)}にあるconfig.json の名前を変えることで解決することがあります(ただし設定がすべてリセットされます)。` |
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.
フォルダを開くボタン(またはリネームして再起動ボタン)を追加してあげると親切なような気がしました。
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.
electron 自体が起動する前なので、ウィンドウを出すのも厳しい気がします。
検証はしてみますが。
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.
https://www.electronjs.org/ja/docs/latest/api/dialog#dialogshowerrorboxtitle-content
app
モジュールでready
イベントが発生する前でも、このAPIは安全に呼び出すことができます。これは、起動の初期段階でのエラーを報告するのによく使用されます。
これが使えそう?
選択はできませんが
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.
あ、僕もちょっと思いました。ダイアログにしたら良さそうだなと。
たしかにdialog.showErrorBox
使えそうですね!!
他にもdialog.showErrorBox
使ってる所あるので参考になるかもです。
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.
dialog.showErrorBox
を出した後に、shell.openPath
や shell.showItemInFolder
でフォルダを開けないか試してみましたが、流石に ready 前なので動かなそうでした。
ひとまず dialog.showErrorBox
にしますね。
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.
個人的には依存増やしてまで(メンテコスト増やしてまで)ディレクトリ開く機能を付けるのはしなくても良いかも?と思いました。
まあ、@raa0121 さんのやりたいように…!
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.
私も同じく、依存を増やすほどじゃないかなと思いました。
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.
どうでしょう? @sevenc-nanashi
まあ良いかと思える感じであればapproveいただけると!!
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.
依存を増やしたくない、なるほど(そういう考えをしたことが無かった)
まぁ大丈夫だと思います。
(ボタンを提案した理由はファイルパスを手打ちしなくてもいいから。エラーダイアログはコピペが出来ないので)
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.
手打ちが面倒なのはめちゃわかります。
そもそも開発者向けには、やはりconfig.jsonをクリアする方法を提供するのが一番いいかなと思いました。
(そしてそれは結構実装が大変。electron-storeの仕様が。。。 😇 )
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.
🚀
レビューありです!! マージします!! |
内容
config.json の validation に失敗した時にエラーが出るが、わかりにくいため、別途ログを出すようにしました。
Discord の経緯 https://discord.com/channels/879570910208733277/893889888208977960/1077566801602424963
折りたたみにエラー例を追加しておきました。