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

support case-insensitive url #34

Merged
merged 2 commits into from Jan 2, 2014
Merged

Conversation

datnguyen0606
Copy link
Contributor

Add an option which allows Warp to support case-insensitive url

e.g www.example.com/SunshineState, www.example.com/sunshinestate

@brendonh
Copy link
Owner

brendonh commented Jan 2, 2014

I think I'm okay with this in principle, although I wonder if there will be any weirdness around locales or unicode collation.

Please remove the setCaseInsensitiveURL method, we don't do setters/getters in general (especially ones with confusingly different capitalization to the variable they set, which is sort of ironic in this context!).

Also, if we're going to .lower() every segment name, shouldn't we also .lower() the keys in .dispatch?

@datnguyen0606
Copy link
Contributor Author

Previously, I thought when people want to add their own customize url, they need to lower() it before adding to .dispatch

e.g runtime.config['warpSite'].resource.putChild(path.lower(), child)

@brendonh
Copy link
Owner

brendonh commented Jan 2, 2014

That seems unnecessarily difficult. We already have our own putChild method on WarpResourceWrapper, so let's make that aware of this flag too.

@datnguyen0606
Copy link
Contributor Author

so let's make that aware of this flag too.

so what do need to do?

@brendonh
Copy link
Owner

brendonh commented Jan 2, 2014

Lower the path in putChild.

@datnguyen0606
Copy link
Contributor Author

I think I already did it datnguyen0606@79aaade

@brendonh
Copy link
Owner

brendonh commented Jan 2, 2014

So you did! Okay

brendonh added a commit that referenced this pull request Jan 2, 2014
Support case-insensitive urls per-resource
@brendonh brendonh merged commit 094f860 into brendonh:master Jan 2, 2014
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

2 participants