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

Update to Rubocop 1.45.1 #1878

Merged
merged 5 commits into from
Feb 19, 2023
Merged

Update to Rubocop 1.45.1 #1878

merged 5 commits into from
Feb 19, 2023

Conversation

kmuto
Copy link
Owner

@kmuto kmuto commented Feb 19, 2023

1.45.1追従をしてみます

@kmuto kmuto requested a review from takahashim February 19, 2023 03:22
@kmuto
Copy link
Owner Author

kmuto commented Feb 19, 2023

@takahashim
rubocop 1.45.1追従をしてみました。

  • review.gemspec

    • gem.test_filesは有害ということで削除しています
    • gem.filesで「全部」になっていたのからとりあえずtestを消しました。が、これでも.git*とか余計なものがいろいろ入るな…という印象です。明示的にincludeするパス並べたほうがよい?
  • .rubocop

    • Performance/MapCompact はあまり好みでなかったのでfalseにしています
    • Style/FetchEnvVar はそのほうがいいことってなんかあるんですかね…。とりあえずfalse
  • コードまわり

    • 存在テストするくらいならmkdir_p, rm_rfをそのまま使えということなので、従ってみています。

@takahashim
Copy link
Collaborator

ありがとうございます、確認してみましたがPerformance/MapCompactは有効化してもいいんではないかと思いました。flat_mapメソッドでやってることは明確ですし、複数行の場合のmap do 〜 end.compactみたいな書き方はいまいち分かりやすくないですし。

Style/FetchEnvVarの方は、ENV['FOO'] || 'bar'みたいな記述が散見されてる場合はENV.fetchで統一するのも良いかと思うんですが、そういうのは使ってないのでdisableのままで良さそうです。

@kmuto
Copy link
Owner Author

kmuto commented Feb 19, 2023

そういえば、flat_mapが>=2.7で、もうRuby 2.7以降のビルドにしているので問題ないとは言えるんですが、「素晴しい価値をもたらすから2.7以降でしか動かさなくします」というほどでもないな…というところでも若干躊躇していました。
この先メジャーを上げて完全に2.7以降にするときにenableにする、のほうがよいかも。

@takahashim
Copy link
Collaborator

なるほどです、ではこのままにしておきましょうか。
あとで気が向いたら互換レイヤーでも作るかもしれませんが、そのときはそのときで。

@kmuto kmuto merged commit 1f3a312 into master Feb 19, 2023
@kmuto kmuto deleted the rubocop-1_45_1 branch February 19, 2023 06:16
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