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 amphtml URL for '?date=YYYYMMDD' style diary #614

Merged
merged 3 commits into from Jan 12, 2017
Merged

Conversation

Nyoho
Copy link
Member

@Nyoho Nyoho commented Jan 11, 2017

amp.rb プラグインで amphtml がおかしい問題 #613 を修正していました。
レビューをお願い申し上げます。
spec/plugin/amp_spec.rb は書き方がよくわからなかったのでつけませんでした。すみません。

@machu machu temporarily deployed to tdiary-dev-pr-614 January 11, 2017 11:25 Inactive
@machu machu temporarily deployed to tdiary-dev-pr-614 January 11, 2017 22:56 Inactive
@machu
Copy link
Member

machu commented Jan 11, 2017

ちょっと試せてないのですが、これだとqueryが空の時に?&plugin=ampとなりません?

@Nyoho
Copy link
Member Author

Nyoho commented Jan 11, 2017

本当ですね。直しました!

@machu machu temporarily deployed to tdiary-dev-pr-614 January 11, 2017 23:42 Inactive
@machu
Copy link
Member

machu commented Jan 11, 2017

ありがとうございます!

@machu machu closed this Jan 11, 2017
@machu machu reopened this Jan 11, 2017
@machu machu temporarily deployed to tdiary-dev-pr-614 January 11, 2017 23:44 Inactive
@machu machu merged commit fec59d4 into master Jan 12, 2017
@machu machu deleted the fix-amphtml-url branch January 12, 2017 00:21
@tdtds
Copy link
Member

tdtds commented Jan 12, 2017

MergeじゃなくてSquashしてねって書きにきたら手遅れだった……。
複数コミットで構成されているプルリク、とくに今回のようにバグ修正のための追加コミットが混じっている場合には、MergeじゃなくてSquash and mergeを使ってコミットをまとめるようにしましょう。履歴がすっきりするし、Revertしやすくなるので。

@Nyoho
Copy link
Member Author

Nyoho commented Jan 12, 2017

ごめんなさい、fix-amphtml-url のコミット群自体を rebase -i しておけばよかったです。

@tdtds
Copy link
Member

tdtds commented Jan 12, 2017

いや、プルリク上での議論や編集過程がきちんと残りつつ、マージ時にきれいにできるのがSquash & mergeの良いところなので、そこはあまり気にしなくて良いと思います。かつて良いとされたプラクティスが今も良いわけではないので。

@machu
Copy link
Member

machu commented Jan 12, 2017

確かにPR側のブランチはそのまま残りつつ、master側が綺麗になるほうがいいですね。
もう少しgit力を高めます。指摘ありがとうございます!

@Nyoho
Copy link
Member Author

Nyoho commented Jan 12, 2017

なるほど、理解いたしました。それで他のPRにも oops commit がたくさん残っていつつ、master には残ってないんですね。

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.

None yet

3 participants