Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

データjsonのimportの名前を揃えたい #4410

Closed
kaizumaki opened this issue May 22, 2020 · 12 comments · Fixed by #4480
Closed

データjsonのimportの名前を揃えたい #4410

kaizumaki opened this issue May 22, 2020 · 12 comments · Fixed by #4480
Assignees
Labels
help-wanted 特に助けを必要としているもの improvement 改善や新機能の要望

Comments

@kaizumaki
Copy link
Collaborator

kaizumaki commented May 22, 2020

改善詳細 / Details of Improvement

期待する見せ方・挙動 / Expected behavior

  • importのダブりを解消し、命名規則をパスカルケースにした上で、場合によっては読み込み元のコンポーネントも修正する。

動作環境・ブラウザ / Environment

  • macOS / Windows / Linux / iOS / Android
  • Chrome / Safari / Firefox / Edge / Internet Explorer
@kaizumaki kaizumaki added improvement 改善や新機能の要望 help-wanted 特に助けを必要としているもの labels May 22, 2020
@kaizumaki
Copy link
Collaborator Author

もしほかの場所でも同様のものがあったら教えてください!

@MaySoMusician
Copy link
Contributor

コンポーネント名については Vue.jsのスタイルガイド(日本語版)にある通りパスカルケースで良いと思いますが、それ以外のものに関しては特に規定されていないので、 data/*.json 系ファイルの import は「それがVueコンポーネントではない」という表示の意味も含めて、JSの慣習であるキャメルケースにした方がよいと思います

@munierujp
Copy link
Contributor

munierujp commented May 22, 2020

@MaySoMusician 同じようなコメントをしようと思ったら、先をこされてしまいました 😅
一応、JS/TSにおける一般的な慣習・規則について参考までに載せておきます。

JavaScript Standard Style

Use camelcase when naming variables and functions.

JavaScript Standard Style

Google JavaScript Style Guide

Local variable names are written in lowerCamelCase, except for module-local (top-level) constants, as described above. Constants in function scopes are still named in lowerCamelCase. Note that lowerCamelCase is used even if the variable holds a constructor.

Google JavaScript Style Guide

Airbnb JavaScript Style Guide

Use camelCase when naming objects, functions, and instances.

airbnb/javascript: JavaScript Style Guide

TypeScript Deep Dive

Use camelCase for variable and function names

StyleGuide - TypeScript Deep Dive

@munierujp
Copy link
Contributor

munierujp commented May 22, 2020

ところで、重複しているimport文についてはESLintのno-duplicate-importsルールで機械的に判定できそうです(動作未検証)。

@MaySoMusician
Copy link
Contributor

ところで、重複しているimport文についてはESLintのno-duplicate-importsルールで機械的に判定できそうです(動作未検証)。

下図の通り、重複している箇所に警告が出るようです( 'no-duplicate-imports': 'error' 指定時)
image

@kaizumaki
Copy link
Collaborator Author

データのjsonのimportをキャメルケースにするのはいいのですが、影響範囲が大きいような気がしていました。でも、推奨される方法で揃えてくれるとありがたいです。
重複もeslintでわかるのですね!

@nyagihime
Copy link
Contributor

これ、普通にやってる分にはたぶんESLintでなんとかなると思うのですが、今回のように複数Issueに割れてて且つ新規で、同じファイルを参照してる場合がちと厄介かなと思ってます。
この場合大抵マージのタイミングで判明するので、リリース優先しようとすると、動作に支障がない場合は一旦は重複したままマージしてしまってあとで整理するしかないのかなという気もしています。
滅多に起きないと思いますし

@goki90210
Copy link
Contributor

この重複はまだわかりやすい(私はSonarQubeで検知しました)のですが、

実は重複系は他にもあって、

  • メソッドの名前だけが違って中身は同一(plugins/vue-chart.tsの60-84, 86-108行目)
  • constの定数名は違うが中身は同一(components/cards/UntrackedRateCard.vue 70-84行目)

が今日のマージで検知しています。(この話題が上がっていたので一旦保留してます。)
※静的ソース解析はコピペを許してくれない。

この場合大抵マージのタイミングで判明するので、リリース優先しようとすると、動作に支障がない場合は一旦は重複したままマージしてしまってあとで整理するしかないのかなという気もしています。

私も影響範囲を見極めてマージ後に整理するしかないと思います。

@goki90210
Copy link
Contributor

goki90210 commented May 23, 2020

#4176 も(組み込みオブジェクトとのですが)重複ですね。

もうひとつあるのですが完全に脱線するので #4426 に書きました。
誤りでした。失礼しました。

@mayuritanaka
Copy link
Contributor

着手します

@mayuritanaka
Copy link
Contributor

様々なご指摘がありましたが、影響が大きそうでしたので、ひとまずissueにあった、重複のみ修正しました。よろしくお願いいたします。

@nyagihime
Copy link
Contributor

nyagihime commented May 25, 2020

@mayuritanaka
結構議論が広がってしまってるので、この issue に関してはこれで良いのではないかとおもいます。

@MaySoMusician @munierujp
お二方がおっしゃってる部分は、プロジェクト全体で直していく必要があると思うので、改めて issue わけて議論したほうが良さそうかなと思うのですがいかがでしょうか?

@goki90210 さんがおっしゃってくださってる部分についても、内容的に1本のPRにしないほうがよさそうなので、こちらも改めて issue を立てたほうが良いのではないかなと思ったのですが、いかがでしょう?
(なんか探したら他にもでてきそう…というのもあり

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help-wanted 特に助けを必要としているもの improvement 改善や新機能の要望
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants