online-judge-tools / oj

Tools for various online judges. Downloading sample cases, generating additional test cases, testing your code, and submitting it.
MIT License
1.03k stars 92 forks source link

Change StreamHandler to stdout from stderr #825

Closed ryo-n closed 4 years ago

ryo-n commented 4 years ago

824 対応

StreamHandlerを標準エラー出力から標準出力に変更しました

kmyk commented 4 years ago

@ryo-n

テスト用の統計情報を出力するために oj t --json みたいなオプションを使っていて、これが壊れてしまっています。 stderr を stdout にすることよりもテストを回す方が重要なので、これが壊れないように追加の修正をお願いします。(しんどそうならプルリクエストを放棄して close してもらってもかまいません)

https://github.com/online-judge-tools/oj/blob/3b4820c976e9f235ff87e328d1941ed0102c2e65/onlinejudge_command/subcommand/test.py#L311-L312

https://github.com/online-judge-tools/oj/blob/3b4820c976e9f235ff87e328d1941ed0102c2e65/tests/command_test.py#L18-L20

--json というオプションがあること」も「そのようなオプションが標準入出力を使うこと」もあまり不適切な仕様ではありませんでした。今ではどちらかと言えば消したいと思っています。--json を消して --log-file=path/to.json のような出力先ファイルを指定できる隠しオプション (argparse.SUPPRESS などを使って --help で見えないようになっているとうれしい) を足すように全体を書き直してもらえると理想です。

ryo-n commented 4 years ago

@kmyk

標準出力からjsonデータ部分を抽出する方が影響少なそうだったので、その方法でひとまず対応してみました。 抽出する部分が若干強引なので何か良い方法とかあればアドバイス頂ければと思います。 (jsonデータ構造の正規表現を利用して抽出とかが素直?)

--jsonオプションに関しては消したいとのことなので、--log-file=path/to.jsonの方式もちょっとチャレンジしてみようかと思います。 その場合はtestサブコマンド以外の--jsonオプションも同様に消してしまって問題ないのでしょうか?

ryo-n commented 4 years ago

@kmyk

--json オプションの名前を別のものに変更した上で --help では表示されないようにしてください。

helpで表示しないようにし、オプションの名前を変更しました。

「内部的なテストでしか使われず、もし予期しない入力がされて壊れてもせいぜいテストが落ちるだけ」「--json オプションが消えるとこのコードも消える」という性質のコードなので、実装はこれで問題ないと思います。一方で、なぜこのような「若干強引な」方法が用いられているかについて、コード中にコメントとして簡単に書いておいてほしいです。この issue やプルリクへのリンクを貼る程度でもかまいません。

コメント追加しました

nitpick: snake_case を使ってください。

変数名をsnake_caseに修正しました。

nitpick: 型ヒントを書いておいてほしい

型ヒント追加しました。

このPRではここまでの修正にし、"テスト用output json dataをファイルに保存するように変更する" などの課題を別途作って、 --log-file=path/to.json 対応をしようと思いますがどうでしょうか。

ryo-n commented 4 years ago

はい。「--log-file=path/to.json 対応を別のプルリクにする」のはよい案だと思います

826 でissue立てたのでそちらで対応しようと思います。

ありがとうございます。