search

online-judge-tools / api-client

API client to develop tools for competitive programming
MIT License
63 stars 17 forks source link

Convert `assert` to `SampleParseError` in some situation? #18

Open kawacchu opened 4 years ago

kawacchu commented 4 years ago

online-judge-tools/oj#637

By the way, is assertion in download_sample_cases is right? If assert len(list(case.children)) == 2 is not satified, the page contents are not supported by oj. Therefore, raise SampleParseError is prefer to assertion?

Anyway, this assertion issue handle by other PR. This PR is mergable.

アサーションはバグ検出のために仕込むものですが、assert len(list(case.children)) == 2や他のアサーションに引っかかった時、ojのバグというよりは想定しないイレギュラーな問題ページが存在したということで、SampleParseErrorを投げるのが正しい気がしてきました。どうでしょう？

基本的に私は賛成ですが、 refactoring の複雑さと実用性の向上で天秤にかけているところです。

fukatani commented 4 years ago

イシュー立てありがとうございます。 やるとしたら、assert_or_raise_parser_errorみたいな関数を作ってassertと差し替えることになるんでしょうが、、 該当箇所が多いですし、ぱっと見では見分けが簡単につかないところもありますね。

実績的にassertがfailしてないならそこまで優先順位は高くないのかな。 ただし、ユーザーに見えちゃった部分、見えそうな部分（不安定なコンテスト）があれば教えてください。

SampleParseErrorはojの能力の限界であることが明示されるのに対し、assert落ちはユーザーには何が起きたかわからないため、表に出ちゃうとよくないと思います。

kawacchu commented 4 years ago

手元で比較してみましたが、raise SampleParseErrorを発生させるほうが処理の全体を見通しやすいです。log.errorと併用して、更に賢いエラー報告も期待できます。