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

津山 Dojoの追加とyamltaskの修正 #272

Closed
wants to merge 2 commits into from

Conversation

AnaTofuZ
Copy link
Member

@AnaTofuZ AnaTofuZ commented Mar 1, 2018

目的

  • 津山Dojo の追加においてDojo設定手順書の整合性を確認する

行ったこと

  • 津山を追加するとエラーが発生することを確認
  • stackoverflowの記事に乗っ取り、SELECT setval('dojos_id_seq',(SELECT max(id) FROM dojos)); を実行 (herokuも)
  • order がyamlに吐き出されない問題があった為修正
  • be rails dojos:update_db_by_yaml
  • be rails dojos:migrate_adding_id_to_yaml
  • yamlファイルが適切に吐き出される事を確認しました
    screen shot 2018-03-01 at 18 37 23

AnaTofuZ added 2 commits March 1, 2018 18:24
 coderdojo-japan#267 で`order` を自動追加出来るように設定していたが
yamlファイルを出力するタイミングで,orderを出力するようにした
-  津山の画像と設定を追加した
- order を自動生成する用に設定した為、yamlファイルが一部変更が入っている。
@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

stackoverflowの記事に乗っ取り、SELECT setval('dojos_id_seq',(SELECT max(id) FROM dojos)); を実行 (herokuも)

記事へのリンクが欲しい...!!

@AnaTofuZ
Copy link
Member Author

AnaTofuZ commented Mar 1, 2018

上記リンクにしました!

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

津山 Dojoの追加とyamltaskの修正 #272

yaml task の修正だけ先に確認&マージしたくて、津山 Dojo の追加はそれを確認した後にマージしたいのに、1つのPRに全部まとめられてしまっていて気軽にマージできない... 😭 💦

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

それぞれ目的の異なるPRは、別々に分けると良さそうですね ✅

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

🤔.oO(それともなんらかの事情で1つのPRにまとめざるをえなかったのかな...?)

@AnaTofuZ
Copy link
Member Author

AnaTofuZ commented Mar 1, 2018

なるほど気をつけます!
今回は粒度的に同じでいいかななどと思っていたので、まとめてしまいました。

@AnaTofuZ
Copy link
Member Author

AnaTofuZ commented Mar 1, 2018

言われてみるとPRの中で目的が異なっていたので分けたほうが良かったですね…orz

@AnaTofuZ
Copy link
Member Author

AnaTofuZ commented Mar 1, 2018

一旦このPRをcloseして、PRを作り直したいと思います

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

@AnaTofuZ いえ! 今回は手元で git reset --soft HEAD^ して1コミットずつ確認して、問題なければ再度 pull し直してコミットを戻して、その後マージするかどうか決めようと思います! 👍

@AnaTofuZ
Copy link
Member Author

AnaTofuZ commented Mar 1, 2018

@yasulab お手数かけます… 🙇 🙇

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

とはいえ目的の異なるコミットをそれぞれ確認するために、コミットを分解して1つずつ確認して元に戻してマージするのは面倒なので、次回以降は気をつけてもらえれば 🙏

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

う...、全然関係ないコミットが1つにまとまっていて辛い 😭

  • 津山の画像と設定を追加した
  • order を自動生成する用に設定した為、yamlファイルが一部変更が入っている。

津山を追加するコミットと、yamlファイルが一部変更するコミットの2つに分けるべきですね 😅

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

@AnaTofuZ こちらも読んでおくと参考になるかもです🤔

コミットメッセージアンチパターン: コメント対応
http://koic.hatenablog.com/entry/2016/11/19/000000

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

@AnaTofuZ すみません、修正すべき箇所がやや多そうなので、やっぱりPR閉じて一つずつ作り直した方が良さそうですね >< 💦

@AnaTofuZ
Copy link
Member Author

AnaTofuZ commented Mar 1, 2018

なるほどすいません…。
実はyamlの変更は並び順の細かい変更点は気づかなかったという感じでした。
コミットしているのでメッセージに書く事意識してやるべきですね…!

画像ファイルとyamlのものはyamlを作る流れで出来たものと考えて1コミットにしてしまったので反省です...!

@AnaTofuZ AnaTofuZ closed this Mar 1, 2018
@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

画像ファイルとyamlのものはyamlを作る流れで出来たものと考えて1コミットにしてしまったので反省です...!

作られたタイミングは一緒で、かつ、変更されたファイルも同じですが、それぞれ文脈も変更の意図も全く異なるコミットだと思うので、そういう時は (1つのファイルの中に複数の意味の異なる変更がある場合は) git add -p を使いましょう 👍 ✨

@yasulab
Copy link
Member

yasulab commented Mar 1, 2018

メッセージに書く事意識してやるべきですね…!

ですね! 😸 次に繋げて行きましょう! 🏃💨

急ぎではないので来週とかでも大丈夫です 🙆(もし僕の手が空いていたら、僕が引き継いで追加することもできそうです 📝 💨 )

@yasulab
Copy link
Member

yasulab commented Mar 2, 2018

本日はアナグラさんがお休みなので、僕の方で引き継いで PR 作り直しておきました ;) cc/ @AnaTofuZ

  1. Update order column with new yaml migration task Update order column with new yaml migration task #273
  2. 🆕 Add CoderDojo 津山 Add CoderDojo 津山 #274

@AnaTofuZ AnaTofuZ deleted the add_tuyama_dojo branch March 5, 2018 02:30
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