ENH: Redirect github blob URLs to raw.github URLs

[westurner]

• Factored out URI value parsing logic
• Compile REGEXes once
• Doctest
• Removed ambiguous form handling logic

[westurner]

@minrk
I started to add a unittest import but instead opted for doctests w/ this patch.

I see:

• https://github.com/ipython/nbviewer/blob/master/Makefile#L45
• https://github.com/ipython/nbviewer/tree/master/js/tests

If I was to write tests (and a tox.ini) for this application, is there a preference for/against:

1. a ./tests directory
2. http://webtest.pythonpaste.org/en/latest/
3. http://flask.pocoo.org/docs/testing/

[minrk]

We are just about to refactor nbviewer entirely (drop flask for tornado, use async, etc.) That WIP also does this redirect. As for tests, a tests subpackage in the nbviewer package would be how we've done things for most projects.

[minrk]

Given the problems with Python 3 and dictionary ordering, in IPython, we have moved away from doctests, as we have found them to be more trouble than they are worth.

[minrk]

I actually misread this - we actually should be avoiding GitHub raw URLs, and redirecting those to GitHub API requests, rather than pushing more requests toward them.

[westurner]

Given the problems with Python 3 and dictionary ordering, in IPython, we have moved away from doctests, as we have found them to be more trouble than they are worth.

Aren't the same problems inherent to unittest ordering? (and PYTHONHASHSEED?)
I sometimes work around this by including three-digit numeric fields in the test method name / key.

[westurner]

We are just about to refactor nbviewer entirely (drop flask for tornado, use async, etc.)

So there will be a new dependency on teh tornado web server?

That WIP also does this redirect. As for tests, a tests subpackage in the nbviewer package would be how we've done things for most projects.

I may be able to find the time to help with writing tests, given a link to a branch/repo.

I actually misread this - we actually should be avoiding GitHub raw URLs, and redirecting those to GitHub API requests, rather than pushing more requests toward them.

Why?

[Carreau]

So there will be a new dependency on teh tornado web server?

Probably yes, Flask is blocking which is annoying for a service like nbviewer where you cannot rely on time of remote to respond.

I actually misread this - we actually should be avoiding GitHub raw URLs, and redirecting those to GitHub API requests, rather than pushing more requests toward them.
Why?

Raw github url are not supposed to be un-changing, changes in github API are planed. Also it help github know why their url is fetched by someone or a remote service, and make optimisation based on that.
Finally using API would trivially allow nbviewer to access private github repo if install locally which interest some peoples.

[westurner]

use async

asyncio (tulip - tulip.http)

• http://code.google.com/p/tulip/
• http://hg.python.org/cpython/file/default/Lib/asyncio/
• https://github.com/fafhrd91/aiohttp#gunicorn-worker (Gunicorn + WSGI)

Tornado

• http://www.tornadoweb.org/en/stable/wsgi.html

[westurner]

Raw github url are not supposed to be un-changing, changes in github API are planed.

Are you suggesting that raw github links are going away? Is there a link describing this roadmap?

So the GitHub API is less stable than the HTTP API, and that's why it is needed to use the API?

Also it help github know why their url is fetched by someone or a remote service, and make optimisation based on that.

They need more than a user agent string to serve comperssed plaintext over HTTPS?

Finally using API would trivially allow nbviewer to access private github repo if install locally which interest some peoples.

Good point. So there are different URL routes in this new version of nbviewer?

Fate of this pull request

That's great that this functionality is in the new version. As it is, the live version of nbviewer does not support this functionality (and recompiles regexes on every view invocation).

Would you like me to:
a) add unittests instead of doctests (maybe with optparse/argparse and a '-t' option)

• inline or separate file?

b) just drop this patch and close the pull request

c) other

[minrk]

Why?

Actually, just because GitHub asked people to stop treating raw.github.com as a CDN, and that is precisely what we are doing. I don't think there is a great risk of raw URLs being unstable.

Fate of this pull request

I think we can merge this as-is, actually. It's strictly an improvement over the status quo. +1 to merge, @Carreau?

[minrk]

Current state of my tornado fiddling is at #120 now.

[Carreau]

Yes +1 to merge as it is strictly an improvement. will look at #120 too.