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

Sites #7

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Sites #7

wants to merge 4 commits into from

Conversation

iwhurtafly
Copy link
Member

SiteモデルにValidationを追加しました。その他、コードに微調整が入っているのは、今回は了承下さい。また、前回strong parametersを検討していた際に追加した「validates_url_format_of.rb」を削除し忘れていました。次回のコミットで削除します。今回のコミット以降は、奇麗な単位でコミットして行けると思います。(次回のコミットには、「validates_url_format_of.rb」を削除したという履歴が残ってしまうでしょうが。。)

最後に2つ質問させて下さい。

・今回、url_is_herokuというメソッドを定義していますが、テストの書き方が分かりませんでした。
 アドバイス頂けますか?
・今回のプルリクエストが取り込まれた場合、今のブランチは削除して、別ブランチで作業ですよね?
 (前にも聞いたかもしれませんが、失念してしまいました。。)

@ppworks
Copy link
Contributor

ppworks commented Sep 8, 2013

・今回、url_is_herokuというメソッドを定義していますが、テストの書き方が分かりませんでした。

url_is_heroku?ですかね。

Resolv.getaddress が肝になっているので、この部分を stub してはどうでしょう?

@ppworks
Copy link
Contributor

ppworks commented Sep 8, 2013

・今回のプルリクエストが取り込まれた場合、今のブランチは削除して、別ブランチで作業ですよね?

ですです!実現したい価値や、機能にフューチャーした内容でブランチを切りましょう。

@iwhurtafly
Copy link
Member Author

・今回、url_is_herokuというメソッドを定義していますが、テストの書き方が分かりませんでした。

url_is_heroku?ですかね。

Resolv.getaddress が肝になっているので、この部分を stub してはどうでしょう?

url_is_heroku?です。stubの件、なるほどですね。今回はこの部分のテストは画面からのテストにしようかと思います。今後、テストを書く時の参考にさせて頂きます。

ですです!実現したい価値や、機能にフューチャーした内容でブランチを切りましょう。

OKです!

先に進みたいので、このプルリクエストをマージしますか?

@ppworks
Copy link
Contributor

ppworks commented Sep 9, 2013

今回はこの部分のテストは画面からのテストにしようかと思います。

テストで何を担保したいかを意識してればよいですが、ここはモデルのテスト欲しいですね。

先に進みたいので、このプルリクエストをマージしますか?

ok!

@ppworks
Copy link
Contributor

ppworks commented Sep 10, 2013

url_is_heroku?
が色々いけてないのでリファクタリングしたいですね。こんど一緒にリファクタリングしますかー

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