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

Raises error if no sample found. #626

Merged

Conversation

fukatani
Copy link
Contributor

@fukatani fukatani commented Dec 2, 2019

When user use download, they expect any samples will be downloaded.
That means, if no samples are downloaded, it is unexpected behavior.
It is user friendly, oj shows error occurred explicitly.

$ oj d https://tdpc.contest.atcoder.jp/tasks/tdpc_contest
[x] GET: https://pypi.org/pypi/online-judge-tools/json
[x] 200 OK
[x] problem recognized: AtCoderProblem.from_url('https://atcoder.jp/contests/tdpc/tasks/tdpc_contest'): https://tdpc.contest.atcoder.jp/tasks/tdpc_contest
[x] load cookie from: /home/ryo/.local/share/online-judge-tools/cookie.jar
[x] GET: https://atcoder.jp/contests/tdpc/tasks/tdpc_contest
[x] 200 OK
[x] save cookie to: /home/ryo/.local/share/online-judge-tools/cookie.jar
[ERROR] Failed to parse sample.

@@ -14,6 +16,9 @@ def snippet_call_download_twice(self, *args, **kwargs):
def test_call_download_invalid(self):
self.snippet_call_download_raises(requests.exceptions.InvalidURL, 'https://not_exist_contest.jp/tasks/001_a')

def test_call_download_no_sample_found(self): # See https://github.com/kmyk/online-judge-tools/issues/625
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#625 の問題は本来は取得に成功してほしいものなので、ここでテストとして使うのは不適切に見えます。後でバグが修正されたときにここのテストが落ちるのはうれしくないです。
別の問題を探すか、もう少しコメントを足したいように思います。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kawacchu Thanks! I applied your proposal.

@fukatani fukatani force-pushed the fix/raise-error-if-no-sample branch from a961053 to fabbc06 Compare December 3, 2019 12:58
@kawacchu
Copy link
Contributor

kawacchu commented Dec 4, 2019

There are two failed tests.

First, test_call_download_hackerrank_hourrank_30_a_system requires signup or login now.
This is not due to this PR. -> Fixed by #630 .

Second, test_call_download_kattis_hello has no sample cases in it. Can you fix this?
Sorry, this is CalledProcessError.
Second, something wrong with test_call_download_kattis_hello.

@kmyk kmyk mentioned this pull request Dec 4, 2019
@fukatani fukatani force-pushed the fix/raise-error-if-no-sample branch from fabbc06 to 0e40e52 Compare December 5, 2019 12:51
@fukatani
Copy link
Contributor Author

fukatani commented Dec 5, 2019

https://open.kattis.com/problems/helloからは元々なにもダウンロードできないのですが、エラーになるようになります。
この動作がエラーかというと正常のような気もするのですが、なにもダウンロードできなかったよーと言ってあげないとユーザーがどこにファイルがあるんだ??と探しまわっちゃうかもしれないので、エラーが出るのを正とすることでどうでしょう?

@kawacchu
Copy link
Contributor

kawacchu commented Dec 5, 2019

この動作がエラーかというと正常のような気もするのですが、なにもダウンロードできなかったよーと言ってあげないとユーザーがどこにファイルがあるんだ??と探しまわっちゃうかもしれないので、エラーが出るのを正とすることでどうでしょう?

I agree your opinion!

Copy link
Contributor

@kawacchu kawacchu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me. Thanks!

@fukatani fukatani merged commit f084863 into online-judge-tools:master Dec 5, 2019
@fukatani fukatani deleted the fix/raise-error-if-no-sample branch December 5, 2019 15:28
@kawacchu kawacchu mentioned this pull request Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants