These are chat archives for scalikejdbc/ja

11th
Oct 2017
kenji yoshida
@xuwei-k
Oct 11 2017 00:32
もし正当な理由ある変更なら、必ずしも互換壊すのがダメなわけでも
kenji yoshida
@xuwei-k
Oct 11 2017 01:45
@katainaka0503 もしくは、もしドキュメントのコード例が現状の最新版の3.1.0でコンパイル通らないなら、ドキュメントか本体かわからないけど、どちらかのバグではあるので、issue報告かpull reqお願いします
katainaka
@katainaka0503
Oct 11 2017 03:13
@xuwei-k 了解いたしました
kenji yoshida
@xuwei-k
Oct 11 2017 04:11
toOptionalOneがどういうAPIなのかよく知らない(たぶん個人的に使ったことない) が、本体側Optionでくくるだけじゃ意味が謎( = 必ずSomeになる?) だから、
ドキュメントのサンプルコードの方を直すべき案件なのだろうか?それともOptionにしたところは、現状ではnullが渡ってくるパターンがある、みたいな話?(まぁ @seratch さんとかに任せよう)
katainaka
@katainaka0503
Oct 11 2017 04:39
必ずSomeになるのでおそらくSomeでくくったほうがいいですね・・・一応修正します。
kenji yoshida
@xuwei-k
Oct 11 2017 05:19
いや、必ずSomeになるなら、Optionで受け取る嬉しさがない(?)から、もしそうなら実装そのままでドキュメントのほう直すべきなのかな?と思ったんですが。 mapがoverrideされてるか何かで、Someの場合もNoneの場合もありえる、なら話は別ですが
もしくは他のAPIとの整合性とか他の要因がなにかあるとか・・・?(それは考えすぎな気がしなくもない)
katainaka
@katainaka0503
Oct 11 2017 08:58
すみません、発言の意図を間違って捉えていました。
katainaka
@katainaka0503
Oct 11 2017 09:05
自分が必ずSomeになると発言したのは自分が送ったPRの中でtoOptionalOneの実装において引数として受け取るto: WrappedResultSet => Option[B]の戻り値にOptionをかぶせているところについての発言でした。
kenji yoshida
@xuwei-k
Oct 11 2017 12:51
ん、なんか話が噛み合ってるような、噛み合ってないような…。同じことの繰り返しになるけど、そのOptionをかぶせたのって、ドキュメントと実装合わせる以外にどういう嬉しさがあるんですか?
それとも “実装は変えずに使う側でSomeでくくったほうがいい” と、思い直した、という意味?
Shohei Kamimori
@jyane
Oct 11 2017 15:04
map の引数の型が Option でないなら、copy(owner = Some(owner)) にすれば良い、ということにはならず、toOptionalOne の実装を変えたほうがメリットがあるということ(?)
katainaka
@katainaka0503
Oct 11 2017 15:16
toOptionalOne()は.one(A(a)).toOptionalOne(B(b)).map().list().apply()というように使用するのですが、現在の実装ではapplyで値を取り出すときに、toOptinalOneの引数の関数がNoneを返す時は、map()の引数の関数の戻り値の型にoneの引数の関数の戻り値をキャストするという実装になっています。
これはmapの引数の関数が(a, b) => a.copy(b = Some(b))というような形式のときには同じ型であるため問題になりませんが、
もっと一般の関数を渡すと実行時例外に遭遇することになります。
katainaka
@katainaka0503
Oct 11 2017 15:25
これはtoOneと同じ実装を使っているためで、toOneの戻り値にnullが帰ってきた際もmapの引数が同様の条件を満たす際には同様の実行時例外が発生します。
toOneの戻り値にnullが帰ってくるような実装をしているときに実行時例外が発生してしまうというのは許容できる気がするのですが、
outer joinをする相手の存在の有無をOptionで表すtoOptionalOneで指定する対象が存在せずtoOptionalOneの引数の関数がNoneを返す際にそのような例外が発生するのは危険だなと思い、また、ドキュメントのとおりにmapにOptionが渡されてくるようなAPIにしてそのようなことが起こらないようにすべきではと思ってのPRでした。
katainaka
@katainaka0503
Oct 11 2017 15:42
toOneの戻り値に→toOneの引数の関数の戻り値にですねすみません
Shohei Kamimori
@jyane
Oct 11 2017 16:04
わかった気がしました。現状のテストでも outer join したときに column が null になるようなテストケースがないのか(?)
ありがとうございます :bow:
katainaka
@katainaka0503
Oct 11 2017 16:11
いや、その outer join したときに column が nullになるケースはあって、mapに入る関数が(a, b) => a.copy(b = Some(b))のケースしか無いため、mapの引数の関数を使わずに代わりにキャストによってなんとかする実装でどうにかなってるケースしか無いのが問題そうです。じゃあPRでそのテスト足せよという話ですねすみません・・・
Shohei Kamimori
@jyane
Oct 11 2017 16:13
あ、僕は吉田さんとは違ってメンバーではないので :bow: