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

slim-lintの4ルールが適用できるか調査し反映する #7395

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

a-terumoto-gs
Copy link
Contributor

@a-terumoto-gs a-terumoto-gs commented Feb 16, 2024

Issue

補足情報のあるissue

概要

slim-lintが指摘するルールで無視する設定になっているもののうち、以下4つのルールについて調査を行いその結果をconfig/slim_lint.ymlに反映させています。

指摘の付くビューファイルがなかったため設定ファイルから該当ルールの記述のみ削除したもの1つ

  • Layout/IndentationConsistency
    一貫性のないインデントをチェックする、コード内のインデントが一貫しているかどうかを確認するルール
    Layout/IndentationConsistency
# bad
class A
  def test
    puts 'hello'
     puts 'world'
  end
end

# good
class A
  def test
    puts 'hello'
    puts 'world'
  end
end

調査の結果、ルールの適用が困難、あるいは適切でないと判断したため据え置きコメント追加したもの3つ

  • Lint/Void
    メソッド内やブロック内で値を返さない式を検出するルール
    Lint/Void
# bad
def some_method(some_array)
  some_array.sort
  do_something(some_array)
end

# good
def some_method
  do_something
  some_num * 10
end

def some_method(some_var)
  do_something
  some_var
end

def some_method(some_array)
  some_array.sort!
  do_something(some_array)
end

変数などの出力を行う際に=を使うslimのルールと相性が悪かったため据え置きとしました

- if object.errors.any?
  .errors
    h3.errors__title
      | 入力内容にエラーがありました
    .errors__body
      ul.errors__items
        - object.errors.full_messages.each do |msg|
          li.errors__item
            = msg  #ここに指摘がつく
  • Metrics/BlockLength
    コード内のブロックの長さを測定し、長大なブロックがないかどうかを評価するルール
    25行以上になると指摘がはいります
    Metrics/BlockLength

調べた結果、slimに対しての対処例は見つからなかったのですが、
形式的にブロックが長くなりやすいRSpec(Railsのテストフレームワーク)では無視する設定にしてることが多いということがわかりました
指摘が付いているファイル数から判断するに、slimはブロックが長くなりやすい傾向があるのではと考え、RSpecと同様無視する設定が妥当なのではと考えています
以下指摘が付いたファイルです

app/views/admin/campaigns/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [49/25]

app/views/admin/campaigns/index.html.slim:49 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [56/25]

app/views/admin/companies/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [60/25]
app/views/admin/users/_table.html.slim:28 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [179/25]
app/views/announcements/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [128/25]
app/views/application/header/_header.html.slim:5 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [30/25]
app/views/articles/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [228/25]
app/views/articles/_recent_articles.html.slim:8 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [38/25]
app/views/articles/_recent_articles.html.slim:10 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/articles/index.html.slim:25 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [49/25]
app/views/articles/index.html.slim:28 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [39/25]
app/views/books/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [67/25]
app/views/buzz/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [37/25]
app/views/courses/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [79/25]
app/views/events/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [142/25]
app/views/hibernation/new.html.slim:18 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [134/25]
app/views/home/_job_seeking_users.html.slim:7 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [79/25]
app/views/inquiries/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [41/25]
app/views/mentor/categories/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [66/25]
app/views/mentor/courses/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [79/25]
app/views/mentor/practices/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [233/25]
app/views/pages/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [154/25]
app/views/practices/_books.html.slim:8 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [36/25]
app/views/practices/_books.html.slim:12 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [29/25]
app/views/products/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [80/25]
app/views/questions/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [97/25]
app/views/questions/_nav_questions.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [27/25]
app/views/regular_events/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [210/25]
app/views/reports/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [208/25]
app/views/reports/_form.html.slim:123 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [27/25]
app/views/retirement/new.html.slim:16 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [80/25]
app/views/shared/_modal_learning_completion.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/survey_questions/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [101/25]
app/views/survey_questions/_form_check_box.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/survey_questions/_form_linear_scale.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [80/25]
app/views/survey_questions/_form_radio_button.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/survey_questions/index.html.slim:43 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [37/25]
app/views/surveys/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [59/25]
app/views/surveys/show.html.slim:34 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [164/25]
app/views/surveys/show.html.slim:36 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [160/25]
app/views/surveys/show.html.slim:99 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [27/25]
app/views/user_sessions/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [49/25]
app/views/users/_form.html.slim:3 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [242/25]
app/views/users/_hibernation_info.html.slim:18 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/users/_inactive_users.html.slim:8 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [36/25]
app/views/users/announcements/_wip_announcements.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/users/events/_wip_events.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/users/form/_os.html.slim:78 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [28/25]
app/views/users/form/_os.html.slim:107 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/users/form/_sns.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [48/25]
app/views/users/pages/_wip_pages.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/users/products/_wip_products.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/users/questions/_wip_questions.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/users/reports/_wip_reports.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/works/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [82/25]
a-terumoto-gs@HW-GS-202:~/bootcamp$ bundle exec slim-lint app/views -c config/slim_lint.yml
app/views/admin/campaigns/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [49/25]
app/views/admin/campaigns/_form.html.slim:23 [W] TrailingWhitespace: Line contains trailing whitespace
app/views/admin/campaigns/index.html.slim:49 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [56/25]
app/views/admin/companies/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [60/25]
app/views/admin/users/_table.html.slim:28 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [179/25]
app/views/announcements/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [128/25]
app/views/application/header/_header.html.slim:5 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [30/25]
app/views/articles/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [228/25]
app/views/articles/_recent_articles.html.slim:8 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [38/25]
app/views/articles/_recent_articles.html.slim:10 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/articles/index.html.slim:25 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [49/25]
app/views/articles/index.html.slim:28 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [39/25]
app/views/books/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [67/25]
app/views/buzz/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [37/25]
app/views/courses/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [79/25]
app/views/events/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [142/25]
app/views/hibernation/new.html.slim:18 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [134/25]
app/views/home/_job_seeking_users.html.slim:7 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [79/25]
app/views/inquiries/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [41/25]
app/views/mentor/categories/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [66/25]
app/views/mentor/courses/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [79/25]
app/views/mentor/practices/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [233/25]
app/views/pages/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [154/25]
app/views/practices/_books.html.slim:8 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [36/25]
app/views/practices/_books.html.slim:12 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [29/25]
app/views/products/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [80/25]
app/views/questions/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [97/25]
app/views/questions/_nav_questions.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [27/25]
app/views/regular_events/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [210/25]
app/views/reports/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [208/25]
app/views/reports/_form.html.slim:123 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [27/25]
app/views/retirement/new.html.slim:16 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [80/25]
app/views/shared/_modal_learning_completion.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/survey_questions/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [101/25]
app/views/survey_questions/_form_check_box.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/survey_questions/_form_linear_scale.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [80/25]
app/views/survey_questions/_form_radio_button.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/survey_questions/index.html.slim:43 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [37/25]
app/views/surveys/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [59/25]
app/views/surveys/show.html.slim:34 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [164/25]
app/views/surveys/show.html.slim:36 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [160/25]
app/views/surveys/show.html.slim:99 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [27/25]
app/views/user_sessions/_form.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [49/25]
app/views/users/_form.html.slim:3 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [242/25]
app/views/users/_hibernation_info.html.slim:18 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/users/_inactive_users.html.slim:8 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [36/25]
app/views/users/announcements/_wip_announcements.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/users/events/_wip_events.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [34/25]
app/views/users/form/_os.html.slim:78 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [28/25]
app/views/users/form/_os.html.slim:107 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [33/25]
app/views/users/form/_sns.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [48/25]
app/views/users/pages/_wip_pages.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/users/products/_wip_products.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/users/questions/_wip_questions.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/users/reports/_wip_reports.html.slim:1 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [31/25]
app/views/works/_form.html.slim:2 [W] RuboCop: Metrics/BlockLength: Block has too many lines. [82/25]

また、ブロックの途中に空行をいれたりして結果が変わるか確認してみましたが、slimはインデントでブロックを判断しているため空行では効果がなく、効果のある対処法はリファクタリングだろうという結論になりました。
しかし、指摘が付いてるファイルの数を考えると現実的ではないため、今回は据え置きとしました

  • Style/FrozenStringLiteralComment
    Rubyスクリプト内で# frozen_string_literal: trueコメントを使用することを推奨するルール
    # frozen_string_literal: trueコメントを付けることで文字列リテラルがフリーズされてパフォーマンスが上がる場合がある
    Style/FrozenStringLiteralComment

このルールを適用するとapp/views以下に置かれているおらくすべてのファイルに対して指摘が付きます。
↓その一部です

app/views/works/_form.html.slim:1 [W] RuboCop: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
app/views/works/_work.html.slim:3 [W] RuboCop: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
app/views/works/edit.html.slim:1 [W] RuboCop: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
app/views/works/new.html.slim:1 [W] RuboCop: Style/FrozenStringLiteralComment: Missing frozen string literal comment.
app/views/works/show.html.slim:1 [W] RuboCop: Style/FrozenStringLiteralComment: Missing frozen string literal comment.

# frozen_string_literal: trueコメントをつけることの影響を調査したところ、該当ファイルのRubyのコードの中に文字列リテラルに破壊的な操作を行う箇所があった場合、その操作が許されなくなるためうまく動かなくなる可能性があることがわかりました。
したがって、機械的にすべてのファイルにこのコメントを追加するのは不具合を発生させる原因になりかねません。
しかし、すべてのコードを確認し破壊的操作の有無を確認してからコメント追加をするのも対象のファイル数的に現実的ではありません。

また、slim-lintのデフォルト設定だとそもそもこのルール除外する設定にしているみたいです。
https://github.com/sds/slim-lint/blob/main/config/default.yml#L79
slimでの例は見つからなかったのですが、erbでは無効化している例が1つですが見つかりました。
https://zenn.dev/junara/articles/efa04ce1ab02c9

以上のことからこのルールについても今回は据え置きとしました

変更確認方法

1.feature/apply-4-slim-lint-rules-survey_resultをローカルに取り込む
2.bundle exec slim-lint app/views -c config/slim_lint.ymlを実行する
3. 指摘がないことを確認する

Screenshot

機能の変更はないので見た目の変化はありません

@a-terumoto-gs a-terumoto-gs self-assigned this Feb 16, 2024
@a-terumoto-gs a-terumoto-gs changed the title 「Layout/IndentationConsistency」ルール削除、他コメント追加 slim-lintの4ルールが適用できるか調査し反映する Feb 16, 2024
@a-terumoto-gs a-terumoto-gs marked this pull request as ready for review February 16, 2024 09:41
@a-terumoto-gs
Copy link
Contributor Author

@yocchan-git
お疲れ様です。
急ぎではありませんので、お手すきの際にこちらのレビューをお願いしてもよろしいでしょうか?
よろしくお願いいたしますm(_ _)m

Copy link
Contributor

@yocchan-git yocchan-git left a comment

Choose a reason for hiding this comment

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

確認しました!OKです🎉

  • 信頼できるサイトのリンクを貼ってある
  • ログを書いている
  • なぜその判断かの経緯もしっかり書いてある

など、丁寧に書いてあったので負担なくレビューできました。🙇‍♂️
対応ありがとうございました🙏

@a-terumoto-gs
Copy link
Contributor Author

@yocchan-git
レビューありがとうございました!
コメントもありがとうございます(^^♪

@a-terumoto-gs
Copy link
Contributor Author

@komagata
お疲れ様です。
チームメンバーの方にapproveいただいたのでレビューをお願いしたいです。
よろしくお願いいたします!

@@ -26,11 +25,11 @@ linters:
- Layout/MultilineOperationIndentation
- Layout/ParameterAlignment
- Layout/TrailingEmptyLines
- Lint/Void
- Metrics/BlockLength
- Lint/Void #除外検討したが見送り
Copy link
Member

Choose a reason for hiding this comment

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

このタイプのコメントがこの設定ファイル中で本当に機能するのかが気になりました。

また、コメントはだいたい#から1スペース開いてることが多いのでそれに合わせた方がいいかなと思いました。

Suggested change
- Lint/Void #除外検討したが見送り
- Lint/Void # 除外検討したが見送り

Copy link
Contributor Author

@a-terumoto-gs a-terumoto-gs Feb 19, 2024

Choose a reason for hiding this comment

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

@komagata
.ymlの形式のファイルのコメントの仕様について調べてみました。
いくつかサイトを見たところ、コメントの前に#を付ける形でよいということでした。
#のあとからその行の末までがコメント扱いになります。
ということで、コメントのタイプはそのままで据え置き、#の後に1スペース追加のみしております。
確認よろしくお願いいたしますm(__)m偽t

以下確認したサイト例です
https://www.redhat.com/ja/topics/automation/what-is-yaml
https://apidog.com/jp/blog/how-to-write-a-yaml-file/

@@ -25,11 +25,11 @@ linters:
- Layout/MultilineOperationIndentation
- Layout/ParameterAlignment
- Layout/TrailingEmptyLines
- Lint/Void #除外検討したが見送り
- Metrics/BlockLength #除外検討したが見送り
- Lint/Void # 除外検討したが見送り
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Lint/Void # 除外検討したが見送り
- Lint/Void # 除外検討したが見送り

Copy link
Contributor Author

@a-terumoto-gs a-terumoto-gs Feb 20, 2024

Choose a reason for hiding this comment

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

@komagata
確認が甘く、修正の爪が甘くて申し訳ございません。
3つとも#の前の不要なスペース1つを削除しました。

Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

確認させて頂きました。OKです〜🙆‍♂️

@komagata komagata merged commit a364995 into main Feb 22, 2024
3 checks passed
@komagata komagata deleted the feature/apply-4-slim-lint-rules-survey_result branch February 22, 2024 02:12
@github-actions github-actions bot mentioned this pull request Feb 22, 2024
19 tasks
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.

3 participants