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

Added support for a prefixed base url that does not contain a trailing slash #1639

Closed
wants to merge 2 commits into from

Conversation

nek4life
Copy link
Contributor

This was simply a behavior I wanted so I went through to see if I could accomplish hacking on pyramid to make it so. Pyramid's current behavior when supplying an empty string is to match the root of the domain I think this behavior should be carried through to routes with a route prefix. At first I wanted to match both an empty string and '/' but that would break a lot of existing applications that depend on the current behavior.

Right before submitting this patch I looked and found a long discussion for issue #406 which in hindsight I should have read prior to doing this work. I will go back and read through the discussion to see if what I changed makes sense based on the content there.

@mmerickel
Copy link
Member

This is in the discussion for Pyramid 2.0 and I personally think this is a good approach despite it being somewhat bw incompatible. That being said you haven't signed the CONTRIBUTORS.txt agreement yet so I'm closing this for now. If you want to push such a commit we can keep it open.

@mmerickel mmerickel closed this Apr 4, 2017
@nek4life
Copy link
Contributor Author

nek4life commented Apr 4, 2017

Hey @mmerickel I'd definitely like to see this functionality for Pyramid 2.0. Are you suggesting merging this prior to that despite it being somewhat bw incompatible or waiting until 2.0? The only reason I ask is I tried to make this semi backwards compatible while getting the new functionality, but I'm wondering if it's for Pyramid 2.0 then would you consider going full backwards incompatible and this merge only being a half measure?

In either case. If this is to be merged I've updated my branch to include my signature to the contributors agreement. I can open a new pull request, just let me know.

Thanks for following up.

@mmerickel
Copy link
Member

@nek4life If you push to your branch it should update this PR automatically unless Github has gotten confused.

The only incompatibility that I see is in what '' means for config.add_route. I was avoiding revisiting this issue myself until 2.0 probably but noticed that the PR couldn't be used as is without your signature while I was pruning the backlog.

@mmerickel mmerickel reopened this Apr 4, 2017
@mmerickel
Copy link
Member

Ok it found the commit after I reopened the PR.

@nek4life
Copy link
Contributor Author

nek4life commented Apr 4, 2017

I agree in it's current form setting an empty string isn't immediately obvious. However, it was in my list of things to try since when I added '/' it appended a slash to the end of the url. So FWIW my first instinct was to replace the slash with an empty string expecting for it to be removed when I did so. At least this way there is an option to remove it from url when the route is prefixed by a group.

Thanks for opening the request back up.

@mmerickel
Copy link
Member

replaced by #3415

@mmerickel mmerickel closed this Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants