api-client
api-client copied to clipboard
Convert `assert` to `SampleParseError` in some situation?
online-judge-tools/oj#637
By the way, is assertion in
download_sample_casesis right? Ifassert len(list(case.children)) == 2is not satified, the page contents are not supported byoj. Therefore,raise SampleParseErroris prefer to assertion?Anyway, this assertion issue handle by other PR. This PR is mergable.
アサーションはバグ検出のために仕込むものですが、
assert len(list(case.children)) == 2や他のアサーションに引っかかった時、ojのバグというよりは想定しないイレギュラーな問題ページが存在したということで、SampleParseErrorを投げるのが正しい気がしてきました。どうでしょう?
基本的に私は賛成ですが、 refactoring の複雑さと実用性の向上で天秤にかけているところです。
イシュー立てありがとうございます。
やるとしたら、assert_or_raise_parser_errorみたいな関数を作ってassertと差し替えることになるんでしょうが、、
該当箇所が多いですし、ぱっと見では見分けが簡単につかないところもありますね。
実績的にassertがfailしてないならそこまで優先順位は高くないのかな。 ただし、ユーザーに見えちゃった部分、見えそうな部分(不安定なコンテスト)があれば教えてください。
SampleParseErrorはojの能力の限界であることが明示されるのに対し、assert落ちはユーザーには何が起きたかわからないため、表に出ちゃうとよくないと思います。
手元で比較してみましたが、raise SampleParseErrorを発生させるほうが処理の全体を見通しやすいです。log.errorと併用して、更に賢いエラー報告も期待できます。