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

Convert assert to SampleParseError in some situation? #18

Open
kawacchu opened this issue Dec 19, 2019 · 2 comments
Open

Convert assert to SampleParseError in some situation? #18

kawacchu opened this issue Dec 19, 2019 · 2 comments
Labels
good first issue Good for newcomers

Comments

@kawacchu
Copy link
Contributor

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
Copy link
Contributor

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

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

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

@kawacchu
Copy link
Contributor Author

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

@kmyk kmyk transferred this issue from online-judge-tools/oj May 2, 2020
@kmyk kmyk added the good first issue Good for newcomers label Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants