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

template内での ?? と ?. の利用をeslintで制限する #1010

Merged

Conversation

k-chop
Copy link
Contributor

@k-chop k-chop commented Nov 4, 2022

内容

何度かコメントされているのを観測したのでeslintで弾くようにしてみました。
eslint-plugin-vueの vue/no-restricted-syntax ルールを使っています。
これはtemplate内でのみ有効なので <script> 内や他には影響を及ぼしません。
ref: https://eslint.vuejs.org/rules/no-restricted-syntax.html

単に禁止だと理由が伝わらないのでカスタムメッセージを設定しました(スクリーンショット参照)

スクリーンショット・動画など

image

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!
こういった設定もあるんですね!!

"vue/no-restricted-syntax": [
"error",
{
selector: "LogicalExpression[operator=??]",
Copy link
Member

Choose a reason for hiding this comment

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

ついでになのですが、同様にオプショナルチェーン?.)もエラーにできそうでしょうか 🙇
たしかこちらも同様にsyntax highlightが崩れるので・・・。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

optional chainingも MemberExpression[optional=true] で問題なく対応できそうでした!
利用箇所が10ヶ所ほど残っていたので合わせて修正しますね

Copy link
Contributor Author

Choose a reason for hiding this comment

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

51acc52 でoptional chainingを禁止し、
5f0db60 で利用箇所を修正しました。シンプルに三項演算子に置き換えています a?.b => a ? a.b : undefined

@k-chop k-chop changed the title template内での??利用をeslintで制限する template内での ?? と ?. の利用をeslintで制限する Nov 4, 2022
@@ -175,7 +175,7 @@
text-color="display-on-primary"
class="q-mt-sm"
@click="
changeHotkeySettings(lastAction, lastRecord)?.then(() =>
Copy link
Contributor Author

@k-chop k-chop Nov 4, 2022

Choose a reason for hiding this comment

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

changeHotkeySettingsの返り値は Promise<void> 確定で
?. は必要なさそうだったので単純に外しました
(下の solveDuplicated も同様)

    const changeHotkeySettings = (action: string, combo: string) => {
      return store.dispatch("SET_HOTKEY_SETTINGS", {
        data: {
          action: action as HotkeyAction,
          combination: combo,
        },
      });
    };

@k-chop
Copy link
Contributor Author

k-chop commented Nov 4, 2022

余談:
https://github.com/vuejs/vue-syntax-highlight
githubのsyntax highlightingにはこれが使われているようですが、2年前から動きがないですね・・・
optional chainingで崩れる件もissue立ってはいるようです

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

approveできてなかったので再LGTM!!

そうなんですよね、issueがそのままなので悲しみがあります・・・。

@Hiroshiba
Copy link
Member

こちらもマージできていませんでした・・・!マージします!!

@Hiroshiba
Copy link
Member

検索用にメモ

Null 合体演算子 (??) Nullish coalescing operator

@Hiroshiba Hiroshiba merged commit 5ec1468 into VOICEVOX:main Dec 4, 2022
@k-chop k-chop deleted the restrict-nullish-coalescing-in-template branch December 5, 2022 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants