Code review comment for lp:~mnordhoff/loggerhead/yui-cdn

Revision history for this message
Matt Nordhoff (mnordhoff) wrote :

Hi,

This adds a "--yui-cdn" option to serve-branches that will make Loggerhead serve the YUI files from Yahoo's CDN on http://yui.yahooapis.com/ instead of its local copy.

I added a yui_url method to BranchWSGIApp, similar to static_url, only...for the YUI files.

I tried it and it works, but that's about it.

Potential issues:

* Where should I have added the argument to BranchWSGIApp.__init__? I put it at the end. Should I have done that at all? Should I have used the 'config' argument or something instead?

* Whenever Loggerhead's copy of YUI is updated to a new version, the URL in BranchWSGIApp.yui_url will also need to be updated. Is anybody going to remember? :P

* I didn't add it to start-loggerhead. Do we care about that anymore?

* I didn't add any tests. To tell the truth, I lost interest while figuring out the config stuff. :P

* When Google's CDN gets a copy of YUI 3, I'll want to add support for that. I'd probably do it by changing the "--yui-cdn" argument to "--yui-cdn={google,yahoo,none}", which would break backwards compatibility. Do you want me to change it to "--yui-cdn={yahoo,none}" now so that won't be an issue?

* It still loads the individual files when using Yahoo!'s CDN. It would be more efficient to load the combined file, but, well, that would take more code. :P

Hmm, I think that's like 2 issues for every line of code I added. Maybe it's not worth it. :P

« Back to merge proposal