#464: breaking changes #484
Conversation
|
テストは落ちていますが、差分はいいと思います。 |
kmyk
force-pushed
the
issue/464
branch
3 times, most recently
from
fb34d18 to
9107721
Compare
|
@fukatani レビューお願いします。
|
| @@ -117,13 +117,13 @@ def from_url(cls, url: str) -> Optional['HackerRankProblem']: | |||
| and result.netloc in ('hackerrank.com', 'www.hackerrank.com'): | |||
| m = re.match(r'^/contests/([0-9A-Za-z-]+)/challenges/([0-9A-Za-z-]+)$', utils.normpath(result.path)) | |||
| if m: | |||
| return cls(m.group(1), m.group(2)) | |||
| return cls(contest_slug=m.group(1), challenge_slug=m.group(2)) | |||
There was a problem hiding this comment.
（PRのスコープ外ですがジャストコメント）csacademyはcontest_nameなのにここがslugなの、なにが違うんだろうと少し気になりました。
There was a problem hiding this comment.
これは HackerRank 側がそう呼んでいるのをそのまま借りてきたためです (修整が落ち着いたらコメント足しておきます)
There was a problem hiding this comment.
なんか前も聞いたかもですね。HackerRankでつかっているならそのままの変数名でいいと思います。
|
|
||
| return None | ||
|
|
||
| @classmethod | ||
| def _from_table_row(cls, tr: bs4.Tag, lang: str) -> 'AtCoderContest': | ||
| def _from_table_row(cls, tr: bs4.Tag, *, lang: str) -> AtCoderContestContentPartial: |
There was a problem hiding this comment.
AtCoderContestのクラスメソッドの_fromなのに、ここでAtCoderContestContentPartial型を返すのは少し意外かもしれません。ちゃんと型まで読んでれば大丈夫ですが、なんとなく読んでるとミスります。
AtCoderSubmissionTestSet._from_table_row()が'AtCoderSubmissionTestSet'を返すのはだいぶわかりやすいです。(fromというからには自分自身の型を返すコンストラクタなのかと、)
There was a problem hiding this comment.
今回の変更の表面的なところしかわかっていませんが、
AtCoderContestContentPartial._from_table_rowにするか、
_fromではなくて、extract_contest_content_partialとかそういう名前にするのが良いのかなと思います。
ただ、該当箇所が多いので、この大きくなってしまったPRでやるより今後の課題かもですね。
There was a problem hiding this comment.
この指摘はまったくその通りです。
修整はしたいですが、この部分はひとまず見逃してほしいです。プルリクがすでに大きくなってるのと、他に同時で修整したい部分がある (...Content は NamedTuple でなく class を切るべきだった) ので別のプルリクとして出したいためです。
|
Partialがセッションなしでも取れる情報で、そうじゃないのは取得にセッションが必要な情報、であってます？ |
|
Partial もセッションは必要です。 |
|
とりあえず これらのクラスの役割は「問題に関する (URL だけからでは分からない) データ」を表現することです。しかしこれを1語で表すものはなさそうで、 (ただしこれは変更するにしても次のプルリクにしたいです。) |
ありがとうございます。そういうことであれば、一覧からとれる情報である、と一言コメントがあると私としては嬉しいです。いい場所があれば、、 |
|
名前に関しては答えがないときは仕方がないですね。明確によくなるときは変えればいいんですけど、悩むということはどっちも大差ないということなので、時間や体力を使いすぎるべきところではないかもしれません、kmykさんのお気持ちで最後は決めちゃってください。 Contentは辞書的には広い言葉なんですけど、webの文脈的にはページで表示されている内容みたいな意味があって、悪くない気もします。ただ実際、Contestと見間違えました。 あえて一つ候補を足すとすれば、 ただ、sessionとかrespはpropertyっぽくないんですよね。 |
|
課題は残ってますがどれも別のプルリクにしたいものなので、とりあえずマージしちゃいますね (これはまだ |
#464
放置すると拡大してしまう系の設計ミスなのではやめに修正します。
#461 とかが上手くいくようになるはずです。
(大規模な破壊的変更なので他のプルリクを巻き込んで衝突しないように
developbranch に向けてのプルリクにしてあります)