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

Add missing routing tests for info controller #21776

Merged
merged 1 commit into from
Sep 27, 2015

Conversation

aditya-kapoor
Copy link
Contributor

No description provided.

@rails-bot
Copy link

r? @eileencodes

(@rails-bot has picked a reviewer for you, use r? to override)

@eileencodes
Copy link
Member

@aditya-kapoor thanks for the PR. 😄 I think the commit message should have more detail about why you added these tests - meaning explain what was missing that you wanted to add them.

@aditya-kapoor
Copy link
Contributor Author

@eileencodes thanks for the review...

Should I change the commit message?

This is vaguely related to #21605 where I proposed to remove this index route but this was kept so I thought it made sense to add some tests regarding this.

@eileencodes
Copy link
Member

Yes I'm just asking you to change the commit message. It's better for Rails in the long term to have commit messages that explain why we made changes.

Vaguely related to rails#21605 where I proposed to remove index route since it was redirecting to the 'routes' action,

but this was kept so I thought it made sense to add some tests regarding this.
@aditya-kapoor
Copy link
Contributor Author

@eileencodes I have updated the commit message...:smiley:

eileencodes added a commit that referenced this pull request Sep 27, 2015
Add missing routing tests for info controller
@eileencodes eileencodes merged commit ed3ad64 into rails:master Sep 27, 2015
@eileencodes
Copy link
Member

By the way, I just noticed you had a typo in your test. Fixed in 0ce3b97

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