[review] each_with_progress メソッドを追加 #44
Conversation
|
|
tkawa
force-pushed
the
any-enum-with-progress
branch
from
5c8f134
to
ceeeff9
Compare
一般のenumeratorを受け取れる。
tkawa
force-pushed
the
any-enum-with-progress
branch
from
ceeeff9
to
e378d76
Compare
元々ちゃんとテスト用意されてる箇所で難しくないと思うしテスト追加よろですー。 |
| if target_scope.is_a?(ActiveRecord::Batches) | ||
| target_scope.find_each | ||
| else | ||
| # ActiveRecord由来のfind_eachでないと思われる場合は明示的にenumeratorに変換 |
There was a problem hiding this comment.
これがテストのDekiru::DummyRecordで失敗しないようにするためだけだとしたら、テストの方を直すほうが正攻法な気がしました。
あとでこれ読んでも何のための考慮か理解するのが難しそうだし、実際の利用上はまず必要のない分岐だろうなーと。
There was a problem hiding this comment.
@aki77
今までも find_each がないところで find_each を生やして使っている例があるんじゃないかと思って、テストを通すためもありますが互換性のためです。
仕様を
「target_scopeは『ブロック引数がなければenumeratorを返す』find_eachメソッドを持つ」
に変更するのであればテストもそのように変更しますがそれでいいですか？
（実際のところ分岐せず常に target_scope.enum_for(:find_each) としても問題なさそうですが）
https://github.com/search?q=org%3ASonicGarden+find_each_with_progress&type=code
https://github.com/SonicGarden/planet-producttimes/blob/792fd05ff22880c4816e6aa59b20f540e8e1f8d3/scripts/lib/import_jicfs_codes.rb#L16
https://github.com/SonicGarden/aelz/blob/07bae8c1c1e5a57cf981ba652f788e92bf217a12/script/maintenance/20240206_import_tour_resources.rb#L9
ざっと探すと2例ですがどちらもenumeratorを返すので動作はしそう。
There was a problem hiding this comment.
なるほどー。
手動実行用のスクリプトでしか使わない処理だし、gemの更新案内見逃したとしてもエラーで落ちたら新しいメソッド使うように書き換えればいいだけなので、わざわざ古い仕様を維持する必要はないかなーと思いました。
tkawa
changed the title
tkawa
force-pushed
the
any-enum-with-progress
branch
from
5045fe9
to
6bf3145
Compare
従来の find_each_with_progress の引数 target_scope が持つ find_each メソッドは「ブロック引数がなければ enumerator を返さなければならない」に変更。
tkawa
force-pushed
the
any-enum-with-progress
branch
from
6bf3145
to
8743ee9
Compare
|
@aki77 |
find_each 以外でも find_each_with_progress を使いたいので、一般の enumerator/enumerable を受け取れるようにしました。
名前は変更した。each_with_progressのほうがわかりやすいかも？