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

#355: fix pre.previous_sibling become None Bug at yukicoder #356

Closed
wants to merge 1 commit into from

Conversation

uta8a
Copy link
Contributor

@uta8a uta8a commented Mar 6, 2019

No description provided.

@kmyk
Copy link
Member

kmyk commented Mar 6, 2019

きちんと修正できててよさそうです。
しかし、以下の2点をお願いします。

  1. このプルリクはひとまず閉じて、 git rebase して master へ向けたプルリクとして作り直してほしい
  2. テストの修正をしてほしい

(1) は完全にこちらの都合です。 issue/355 へのプルリクにした判断は間違っていないのですが、CI が回らないことと、ちょうど今 #346 がマージされて直接 master へプルリクを出せるようになったためです。
(CI が実行されてないのは負荷軽減のため実行が master へのプルリクだけに設定されてるからなのですが、プルリク間の衝突の回避のためにブランチが増えてきたので諦めて削除した方がいい気がしてきています。そのうち修正します。)

(2) もかなり罠っぽくて申し訳ないです。
次の @unittest.expectedFailure を削除してもらえばよいです。修正を後回しにするつもりで expectedFailure と書いたのですが、修正できてしまったのでここがWAになってしまうためです。

https://github.com/kmyk/online-judge-tools/blob/db011f2aed9db40b6452eba504d74a996a5c1285/tests/service_yukicoder.py#L40-L45

このテストは $ python3 setup.py test -s tests.service_yukicoder.YukicoderProblemTest かあるいは $ python3 setup.py test で実行できます。このあたりの詳細は CONTRIBUTING.md にあります。

@uta8a
Copy link
Contributor Author

uta8a commented Mar 7, 2019

すみません、不慣れなのでよく分からず確認します。

  1. このプルリクを閉じる
  2. tMasaaa/issue/355 -> kmyk/masterへ直接プルリクを送る(このプルリクの内容はfix pre.previous...と@unittest.expectedFailureの削除をひとつにまとめておく)。このとき、git rebase masterして現在のmasterの状態からブランチを生やした形にする。

以上の手順で行うという理解でよろしいでしょうか?

@kmyk
Copy link
Member

kmyk commented Mar 7, 2019

はい。合っています。

このプルリクを閉じるのはGitHubがプルリクの送り先を変更する機能を持たないため、rebaseするのはプルリクに関係ないコミットを除いて見やすくするためです。
ついでに、 (もう解決してそうですが) 競プロerが git を勉強するときは「git データ構造」とかで検索して https://www.kaitoy.xyz/2015/12/27/git-repository/ https://www.kaitoy.xyz/2017/06/10/git-rebase/ みたいなのを読むのが楽なのでおすすめです。

@uta8a uta8a closed this Mar 7, 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.

2 participants