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

Fix the bug of submit subcommand about Python and AtCoder's language update #718

Merged
merged 7 commits into from
Apr 7, 2020

Conversation

kmyk
Copy link
Member

@kmyk kmyk commented Apr 5, 2020

AtCoder の言語更新で Python コードの提出言語判定が壊れたので修正します。
https://atcoder.jp/contests/judge-update-202004

もともと微妙にバグってたのが、Python (3.8.2) が部分文字列として 32 を含むので Python 3 かつ Python 2 の判定になって顕在化しました。

@fukatani
Copy link
Contributor

fukatani commented Apr 5, 2020

language update contestの問題を使ったテストケースを足すべきかもです。

@kmyk
Copy link
Member Author

kmyk commented Apr 5, 2020

language update contestの問題を使ったテストケースを足すべきかもです。

これはなぜですか? そのテストケースにはどのような利点がありますか? それは実装の手間よりも大きいですか?

@kmyk
Copy link
Member Author

kmyk commented Apr 5, 2020

POJ が落ちてるな

@fukatani
Copy link
Contributor

fukatani commented Apr 5, 2020

これはなぜですか? そのテストケースにはどのような利点がありますか? それは実装の手間よりも大きいですか?

Python 3.8.2をサポートしているコンテストが今他になく、
「今正しく実装されていても私とかが後にバグをいれ込んだりして、今のテストケースだと動いているがlanguage updateで動作しないことが発覚しユーザーに迷惑をかけた」とならないようにするためです。

@kmyk
Copy link
Member Author

kmyk commented Apr 5, 2020

まともな返答が帰ってきてしまったので諦めてテストを書きます

@kmyk
Copy link
Member Author

kmyk commented Apr 5, 2020

書きました。50分かけて書いたまともなやつです

@kmyk
Copy link
Member Author

kmyk commented Apr 6, 2020

手元で通ったテストがなぜか落ちてて、よく見ると過去の私の実装が運良く動いてただけの残念コードでした。直しました。純粋な処理のはずなのに出力が一定でないのはかなりだめ。テスト書いててよかった。

@kmyk
Copy link
Member Author

kmyk commented Apr 6, 2020

@fukatani けっこう修正をしたのでもう一度確認してほしい。しかもできれば早めがよい (今週末までに更新したいので)。申し訳ないがよろしくお願いします 🙇

@@ -156,13 +156,71 @@ def select_ids_of_matched_languages(words: List[str], lang_ids: List[str], langu
return result


def is_cplusplus_description(description: str) -> bool:
return 'c++' in description.lower() or 'g++' in description.lower()
Copy link
Contributor

Choose a reason for hiding this comment

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

ここにclangがないのはなにか理由があるでしょうか?

Copy link
Member Author

Choose a reason for hiding this comment

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

以下のふたつが理由です。分かりにくいしコメントを足しておきます。

  • clang だと C (Clang) とかを巻き込んでしまう
  • C++ (Clang)clang++C++g++ を部分文字列として含む

@@ -172,70 +230,72 @@ def guess_lang_ids_of_file(filename: pathlib.Path, code: bytes, language_dict, c
if ext in ('cpp', 'cxx', 'cc', 'C'):
log.debug('language guessing: C++')
# memo: https://stackoverflow.com/questions/1545080/c-code-file-extension-cc-vs-cpp
lang_ids = list(set(select('c++', lang_ids) + select('g++', lang_ids)))
lang_ids = list(filter(lambda lang_id: is_cplusplus_description(language_dict[lang_id]), lang_ids))
Copy link
Contributor

Choose a reason for hiding this comment

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

あまり理解できてないけど、何回もfilter(lambda lang_id: is_cplusplus_description(language_dict[lang_id])が出てきて何回もlang_idsが再代入されるのが不思議です。

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. filter(lambda lang_id: is_cplusplus_description(language_dict[lang_id]) はちょうど 1 回しか出現していないはずです。
  2. lang_ids への代入を複数回するのは、この関数が複数の条件を用いた絞り込みをしているためです。

Copy link
Contributor

@fukatani fukatani Apr 7, 2020

Choose a reason for hiding this comment

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

# compilerとか# versionなどコメントはあるべきところに書かれてるのでわかってきました。
仕方ないのかな。

Copy link
Contributor

Choose a reason for hiding this comment

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

沢山あるのはfilter(lambda lang_id: parse_cplusplus_compiler(language_dict[lang_id])の方ですね。

@@ -0,0 +1,404 @@
import pathlib
Copy link
Contributor

@fukatani fukatani Apr 7, 2020

Choose a reason for hiding this comment

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

lang_idの獲得に失敗した時のテストも入れたほうがいいと思いますが、今回のリリースに含めるか、TODOにするかはお任せします。

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO にしておきたい。ないよりある方がましなのは分かるが、このテストを書いておいたことで救われるという場面があまり想像できないためです。

@fukatani
Copy link
Contributor

fukatani commented Apr 7, 2020

関数が大きくなってきて、メンテが辛いコードになってきましたね。
特にsubmitguess_lang_ids_of_fileが大きすぎます。

コメントは書かれているのでなんとなくわかってはきました、雰囲気はいいと思います。

多分コメントになっている# interpreterみたいなところは関数に抽出したほうがいいと思うのですが、
このPRの差分がこれ以上大きくなるのもあまりよくないので、
今週とりあえずリリースして、時間をかけてリファクタリングするという形になるでしょうか。
(リファクタリング作業に興味はありです。やれたとして来週以降になりますが、)

@kmyk
Copy link
Member Author

kmyk commented Apr 7, 2020

今回はバグ修正なのでリファクタリングには踏み込みません。
(まだ急いでリファクタリングする段階ではなさそうに見えるけど、やってもらえるならうれしいです。)

@fukatani
Copy link
Contributor

fukatani commented Apr 7, 2020

久しぶりに読んだらそこそこ複雑で、実際バグも複数混入してたということで、読みやすくできるならしたいです。

@kmyk kmyk merged commit 47476a8 into master Apr 7, 2020
@kmyk
Copy link
Member Author

kmyk commented Apr 7, 2020

owner 権限で CI まわりの設定変更してマージしました

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.

2 participants