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

urlencode path values for url() #150

Closed
wants to merge 1 commit into from
Closed

urlencode path values for url() #150

wants to merge 1 commit into from

Conversation

benallfree
Copy link
Contributor

Fixes #146

@kyranb
Copy link

kyranb commented Jan 9, 2016

Thanks! 🙇

@tabennett
Copy link
Contributor

While this may be a quick fix for the problem, it's really not something that I want merged into the codebase. Adding more and more parameter passing into these methods of these classes isn't really a scalable solution; There's no possible way to cover all scenarios that people are going to come up with. I would much rather prefer the use of inheritance (for the interpolator) to fix this issue.

We already have the ability to bind implementations for an interpolator (so that users can substitute their own interpolator implementations). Taking that a step further, because we only need perform that check and url encoding if we're interpolation a url, why can't we have two interpolators (1 for paths, 1 for urls) that both extend the base interpolator (for shared functionality) but have different implementations (the url interpolator will encode values).

@benallfree
Copy link
Contributor Author

Good idea, I'll look at a refactor!

@benallfree
Copy link
Contributor Author

I think this is fixed by #167 and isn't needed anymore.

@benallfree
Copy link
Contributor Author

Closing, I think it's not needed.

@benallfree benallfree closed this Jan 25, 2018
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