-
Notifications
You must be signed in to change notification settings - Fork 886
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
Make Proutes *AMAZING*: Fix checks for @wsgiapp2, MultiView, and add request method #1488
Make Proutes *AMAZING*: Fix checks for @wsgiapp2, MultiView, and add request method #1488
Conversation
@mmerickel @mcdonc Please review. Just realized I probably broke py26 support but I'll fix that :) |
I'm on my phone so didn't review thoroughly but maybe you want to mention in the PR title or description that this is for |
Now we're talking :) |
@mmerickel Don't forget to review this amazing PR |
This is cool; maybe my route and view names are too long, because one negative is that I had to shrink my font size way down in order to fit into the width of my laptop screen. On the plus side, I might be able to get IT to get me a bigger monitor to run proutes. |
If |
Why is the change so big if all you did was add a |
E.g.: $ proutes development.ini --glob='user*' Name Pattern View Method ---- ------- ---- ------ user /profilesvc/v1/user/{user_id} pyramid.config.views.<pyramid.config.views.MultiView object at 0x106f72bd0> * user /profilesvc/v1/user/{user_id} cornice.service.wrapper GET,HEAD
|
@msabramo The reason the diff is so big is because finding the request method is not completely straightforward, it can be defined at the route level, view level, be a list of methods, a single string, or While working on the request method I came up with some complex routing examples that exposed bugs in the original implementation as well so I fixed those as I went along. |
Maybe if it's hard to reliably get the the method, that's an indication that something needs to be fixed in the API that proutes is using? Fixing routing bugs might be another PR as well? |
…rt_more_features_in_routes
…rt_more_features_in_routes
Ok, finally took a look. I don't have much to say. This is awesome. Could the |
@mmerickel Was just trying to be consistent with things like |
…rt_more_features_in_routes Conflicts: CHANGES.txt
I guess I'd vote for supporting both then! |
I don't know about supporting both of them. That probably will make things confusing and possibly harder to maintain. I think I'd stick with the newline approach as that seems to be more typical of what is used in PasteDeploy ini files, including @sontek: This has merge conflicts that need to resolved, even if @mmerickel agrees with sticking with the newline approach. |
…rt_more_features_in_routes Conflicts: CHANGES.txt
I fixed the conflict that YOU caused! ;). I'm also not really wanting to support csv as all because its confusing to have multiple ways of doing something. I would think it would only make sense if we also support csv in things like includes and tweens |
I'm not sure why this feature is being compared to these other things that have only one way to specify them like |
Well yeah but does pip use a csv command line argument or ini setting? I can't think of one. I feel like it's more typical for CLI options to repeat the option than to have a single one with a CSV value. But even if there is a ini file option and an equivalent CLI option, I feel like each one should use the one natural syntax for that medium and for ini files I think that's usually newlines (though I will admit that logger keys is a CSV but I think that's rarer). I don't think it's necessary for each to support the format of other |
This isn't separate features though like every other example (multiple options on the cli, pyramid.includes, kotti.configuration, pipeline, etc). This is a format string. I think logically they should be in a csv format rather than separated by newlines because that's describing the tabular format of the output on the console. |
Pip is an interesting example. AFAICT, |
Interesting. What if it were a format string? E.g.:
|
Yeah I can definitely see this evolving a bit, it's obviously a tiny format dsl. Hopefully we don't have to take it any further than the csv columns though. The CSV does indicate columns somewhat well. |
Ok, I'm going to kill newline support and only do CSV. It'll be consistent between the ini and the cli |
I asked @bertjwregger to be a sane third party if you want him to as well since I realize I'm being like ultra pedantic here. Regardless, the PR is great and I'll merge it once a decision is made. |
Yeah, I prefer more input! I don't really care that much either way, I'll use whatever gets merged :) |
@mmerickel Can't even get my name right with the auto-complete ;-) Will take a look at this tomorrow. I like the idea, and I like the output, will do a quick code review as well. |
Crap I was so close! |
@mmerickel @bertjwregeer I noticed that it didn't link so I tried to guess what the actually name was and I couldn't remember either :D |
@mmerickel There, it now supports every way someone might think of using it :) |
Make Proutes *AMAZING*: Fix checks for @wsgiapp2, MultiView, and add request method
Coolest thing ever. Merged. |
Woo hoo! @sontek: I want that
|
Pyramid 1.5.2:
Before this PR (i.e in master but not released):
After:
Outside of the more readable format there is now 2 command line arguments
--glob
and--format
:Glob lets you filter your routes:
Format allows you to choose which columns are visible:
The format flag can also be placed in the ini with the
[proutes]
section:This also fixes #1180