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

Wildcard fallbacks feature #64

Closed
wants to merge 9 commits into from
Closed

Wildcard fallbacks feature #64

wants to merge 9 commits into from

Conversation

msurdi
Copy link
Collaborator

@msurdi msurdi commented Jun 6, 2015

Hi, I'd like to suggest a feature which maybe you can find interesting too.

It would be very useful to have 'wildcard' directories, so that if for given a request like:

GET /api/users/1/profile/

you don't have a file in ./canned/api/users/1/profile/index.get.json then it would look for a file in ./canned/api/users/:id/index.get.json or similar.

This could be extended to support nested wildcards like ./canned/api/customer/:id/invoice/:id/index.get.json and so forth.

I'll try to find some time to start a PoC implementation if you like the idea.

@sideshowcoder
Copy link
Owner

That sounds like a good idea, I would like to stay with the naming conventions if possible which right now uses any for naming whildcards. I'm not sure if that completly makes sense so... I guess if you have a usecase for that you can provide some insight there, the only thing you need to be aware of is that canned right now supports windows as well which means certain characters in directories are not possible, I'm not sure if : is one of those...

Other than that, that sounds like a great idea! If I can be of any help let me know, I'm happy to accept contributions and help where I can along the way.

@msurdi
Copy link
Collaborator Author

msurdi commented Jun 6, 2015

Hi, again, here is a PoC implementation of the feature. I've added a parameter so that if anyone doesn't like "any" or wants to use ":id" as a directory name for avoiding name collisions they can do it.

This implementation is missing some tests and documentation, but I'd like first to hear if you like it or not before investing more time on it. I'd also appreciate some help with these remaining issues, if anyone else in the project is interested in this.

@@ -28,6 +31,7 @@ var dir = ''
, cors_headers = argv.headers
, logger
, cannedDir
, wildcard = argv.wildcard
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's going to be important to have a sane default here. just to keep in mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default is set to 'any' via optimist's default(). Is that enough?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah sorry misread 👍 all good!

@sideshowcoder
Copy link
Owner

I like the general idea a lot, thanks for taking the time, I think seeing it in code made it clearer what you want to get out of it. I think the general approach is good, for maintainance purpose I would prefer to see the functionality in utils to be extracted to some kind of pathMatching module since I feel this gets really complicated now, and in all honesty it is to complicated already, but I didn't refactor earlier sorry for that. I think with pathmatching moved to a module I'd be more than happy to get this in canned, I'm fairly busy now the coming 2 weeks but I'll take some time to review what you got and see where I can help just so you are aware.

Again thanks for taking the time and happy hacking 👍

@msurdi
Copy link
Collaborator Author

msurdi commented Jun 7, 2015

Hi again. I've refactored the path generation code following your suggestion into its own module, which I named 'lookup' because that's what made more sense to me, feel free to remove it to anything else if you find a better name (naming is difficult!!!). I've also added a few more tests to canned.spec and a unit test for the path generation algorithm.

Anything else I can help with so that we get this into canned?

@sideshowcoder
Copy link
Owner

From first glance this looks good, I'll have a more thourogh look tomorrow and give some more feedback but I'm pretty sure this gets in before the end of the week for sure 👍 Thanks for all your work!

@msurdi
Copy link
Collaborator Author

msurdi commented Jun 7, 2015

Good to hear that. Thank you too for making canned open source.

@sideshowcoder
Copy link
Owner

Merged with 897660f thanks so much for all your work! I will release it in the next version, and I hope you don't mind I ajusted the style a little to fit the rest of the project, I don't mind either style but like to have the consistency, sorry hope it does not bother you.

@sideshowcoder
Copy link
Owner

I just realized some help with the docs would be awesome! Do you mind taking a look at the readme? I copied the PR description but more would be better. Thanks so much!

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.

None yet

2 participants