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 for rails 5 - test rails-edge on travis allowing failure #151

Merged
merged 1 commit into from
Jun 19, 2015

Conversation

twalpole
Copy link
Contributor

Fix for rails 5 removal of Route#optional_parts.
Adds rails-edge to appraisal and tests on travis - allowing failure

@bogdan
Copy link
Collaborator

bogdan commented Jun 19, 2015

It is very weird that optional_parts got removed. Are you sure it wasn't renamed?
Please give a link to commit that removes it.

We need to consider why did they remove it. Maybe they think there should be no optional parts any more or other crazy decision.

@twalpole
Copy link
Contributor Author

It was removed here. rails/rails@198b1dd

@le0pard
Copy link
Member

le0pard commented Jun 19, 2015

@bogdan rails/rails#17930

@bogdan
Copy link
Collaborator

bogdan commented Jun 19, 2015

Lets copy the implementation that is there than. path.optional_names.map(&:to_sym). It feels more correct. Not sure if we need to_sym.

@twalpole
Copy link
Contributor Author

It feels more correct, to me, to stay at the higher level of Route rather than drop down into the implementation of path. It's more isolated from any future changes

@bogdan
Copy link
Collaborator

bogdan commented Jun 19, 2015

From the other hand you've assumed that there is no other routes than required and optional. Not required route is always optional might not be true. It suppose to be in any adequate gem. But rails is not so adequate.

@twalpole
Copy link
Contributor Author

Agreed, but since Path#required_names is implemented as names-optional_names it is currently the case, and if that ever changes there are bigger problems. I feel the current proposed fix is easy to read, easy to understand, and decently isolated from underlying implementation changes

@bogdan
Copy link
Collaborator

bogdan commented Jun 19, 2015

Ok, not a big deal to change anyway.

bogdan added a commit that referenced this pull request Jun 19, 2015
fix for rails 5 - test rails-edge on travis allowing failure
@bogdan bogdan merged commit e366e47 into railsware:master Jun 19, 2015
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.

3 participants