-
Notifications
You must be signed in to change notification settings - Fork 310
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
undefinedとnullの厳密比較を禁止する設定を追加 #1585
Conversation
@sevenc-nanashi こちらのeslintを追加する場合、現段階で、 (修正範囲が大きすぎるので細かく対処していった方が良さそうと考えてます。。。) |
うーん、どうなんでしょう? @Hiroshiba 個人的にはmainのCIは全部通るようにしておきたい+最悪cherry-pickした別ブランチからPRしなおせばいいのでこのPRでやっちゃってもいいと思いますが、ヒホさん(Hiroshibaさん)の判断優先でお願いします。 (ちなみに、suggestionの「Commit Suggestion」を押すといい感じにやってくれます、レビューの解決とかCo-Author登録とか) |
プルリクエストありがとうございます!!! エラー量見てみました、確かに凄まじい数ですね・・・! |
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.
ちょっと別のコメントなんですが、nullやundefinedと比較していなさそうなのにエラーになっている例がかなりあるかもしれません。
例えばこちらとか・・・。
https://github.com/VOICEVOX/voicevox/pull/1585/files#file-src-background-ts-L361
(const COMBINATION_IS_NONE = "####";
です)
これ、なんか直感的にかなり難しい問題な気がしました 😇
@Hiroshiba @sevenc-nanashi 見てくださりありがとうございます🙇♂️ また、こちらに関してですが、
自分の方で調べてみてもなぜこれがエラーになるのかの原因を特定することはできませんでした😇 その対処法として今回のissueの対応では、
といった、 nameになるので、
などもエラーになりますが、予約語であり別の制約に引っかかるのでnameにしても問題なさそうと考えています👀 |
おお、なるほどです!!良さそうに感じました! |
これで試してみたのですが、まだ誤検知があるようでした。 便利なあれこれ見つけたのでメモです。 このツールが便利でした。 esquery 検出したい対象と、検出したくない対象のいろいろです。 // 検出対象
const a = 0 === undefined;
const aa = 0 === null;
const fuga = "null";
const a = fuga === undefined;
const b = "fuga" === undefined;
const button = { text: null };
const c = button.text === null;
const d = "fuga" === null;
const e = accentPhrase.pauseMora === null
// 非検出対象
const COMBINATION_IS_NONE = "####";
const b = 0 === COMBINATION_IS_NONE;
const c = loadedHotkey.action === defaultHotkey.action |
(横から突然すみません)
nullとundefinedのASTはそれぞれこんな感じなので、 ASTはここで見れます |
@k-chop おおなるほどです!!!!!詳しくありがとうございます!!! |
@k-chop おぉぉぉぉ!!こんな便利ツールがあるんですね!!! |
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!!!
プルリクエストの Files changed
の下にワーニングが大量に出ていますが、今しばらくは無視する形で良いのかなと思いました!
まだ禁止する(エラーにする)ところまで到達していないので、元のissueはopenのままにしておこうと思います。
@Tksn07 さん
初のプルリクエストありがとうございました!!(ちなみに問題は全くなかったです!)
もしよかったらまたプルリクエストをいただけるととても嬉しいです!
ワーニングを消していくとか、是非・・・!
内容
↓ こちらのissueを修正したPRになります。
eslintにundefinedとnullの厳密比較を禁止する設定を追加しました。
下記のコードを、main.tsなどに張り付けていただければ確認できます。
関連 Issue
ref #1513
スクリーンショット・動画など
その他
初めてOSSにPRを切らせていただきました。
何かやり方が間違っているなどあれば教えていただけると幸いです🙇