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

refactor providers #443

Merged
merged 12 commits into from Apr 28, 2015

Conversation

Projects
None yet
3 participants
@bollwyvl
Contributor

bollwyvl commented Apr 16, 2015

This PR is an implementation of #428:

  • break up handlers.py, mostly into providers/<provider>/handlers.py
  • move provider-specific things into the respective providers
    • clients
    • UR[I|L] rewrites
    • configuration
    • template details
    • tests
  • documentation
    • add README section
    • explain how to extend providers

At the end of this, it should be much clearer how to create a new handler, how to configure existing handlers, etc.

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 17, 2015

Contributor

This is coming along: there is no longer any specific mention of github outside of the github provider other in the python code. I'm going to take a look at what would be entailed in pulling those out as well...

Contributor

bollwyvl commented Apr 17, 2015

This is coming along: there is no longer any specific mention of github outside of the github provider other in the python code. I'm going to take a look at what would be entailed in pulling those out as well...

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 18, 2015

Contributor

I've moved the github/gist-specific template values and error handling out and into the provider. Tests next? The general ecosystem pattern usually has the tests near where the code is, so I guess the providers should get tests...

Contributor

bollwyvl commented Apr 18, 2015

I've moved the github/gist-specific template values and error handling out and into the provider. Tests next? The general ecosystem pattern usually has the tests near where the code is, so I guess the providers should get tests...

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 20, 2015

Member

If you end up writing wholly new tests, might I suggest migrating some to using Tornado's async testing setup? It should make mocking much easier - the current tests are doing integration/functional testing by hitting the webserver like a black box.

Member

rgbkrk commented Apr 20, 2015

If you end up writing wholly new tests, might I suggest migrating some to using Tornado's async testing setup? It should make mocking much easier - the current tests are doing integration/functional testing by hitting the webserver like a black box.

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 20, 2015

Contributor

Just gardening... So haven't written any new tests, just moved the old
ones. I agree though... I'd really like to have per-save test execution,
for which api mocking is a hard requirement! Otherwise any feedback?
On Apr 20, 2015 10:55 AM, "Kyle Kelley" notifications@github.com wrote:

If you end up writing wholly new tests, might I suggest migrating some to
using Tornado's async testing setup
http://www.tornadoweb.org/en/latest/testing.html? It should make
mocking much easier - the current tests are doing integration/functional
testing by hitting the webserver like a black box.


Reply to this email directly or view it on GitHub
#443 (comment).

Contributor

bollwyvl commented Apr 20, 2015

Just gardening... So haven't written any new tests, just moved the old
ones. I agree though... I'd really like to have per-save test execution,
for which api mocking is a hard requirement! Otherwise any feedback?
On Apr 20, 2015 10:55 AM, "Kyle Kelley" notifications@github.com wrote:

If you end up writing wholly new tests, might I suggest migrating some to
using Tornado's async testing setup
http://www.tornadoweb.org/en/latest/testing.html? It should make
mocking much easier - the current tests are doing integration/functional
testing by hitting the webserver like a black box.


Reply to this email directly or view it on GitHub
#443 (comment).

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 20, 2015

Member

I like the organization you've laid out here and am happy you kept the defaults to be what's on the "reference implementation", for the sake of people wanting to contribute to nbviewer.ipython.org and for ease of deployment.

Member

rgbkrk commented Apr 20, 2015

I like the organization you've laid out here and am happy you kept the defaults to be what's on the "reference implementation", for the sake of people wanting to contribute to nbviewer.ipython.org and for ease of deployment.

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 20, 2015

Contributor

Thanks, that was the goal.

This will represent a breaking change, as I didn't shim any of the old locations: should we actually do a major increment to 1.0? Perhaps when this and #421 land, that might be appropriate.

Anyhoo, looks like needs rebase... after that, I'm going to take of the [wip]... don't want to drag this out too long!

Contributor

bollwyvl commented Apr 20, 2015

Thanks, that was the goal.

This will represent a breaking change, as I didn't shim any of the old locations: should we actually do a major increment to 1.0? Perhaps when this and #421 land, that might be appropriate.

Anyhoo, looks like needs rebase... after that, I'm going to take of the [wip]... don't want to drag this out too long!

@bollwyvl bollwyvl changed the title from [wip] refactor providers to refactor providers Apr 21, 2015

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 21, 2015

Contributor

Ok, had enough fun here: ready to review, but certainly open to any other changes. I still don't know how someone coming along would create and set custom configuration values for a custom provider except via environment variables!

Contributor

bollwyvl commented Apr 21, 2015

Ok, had enough fun here: ready to review, but certainly open to any other changes. I still don't know how someone coming along would create and set custom configuration values for a custom provider except via environment variables!

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 21, 2015

Member

What do you mean by:

didn't shim any of the old locations

Are you talking code locations or URL paths?

Member

rgbkrk commented Apr 21, 2015

What do you mean by:

didn't shim any of the old locations

Are you talking code locations or URL paths?

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 21, 2015

Contributor

Code locations! The URL scheme is unchanged! The only thing that should be
any different from outside is that places where something always said
"Github" might say "Gist" instead, as I tried to make the templates more
provider agnostic.

On Tue, Apr 21, 2015 at 12:03 PM Kyle Kelley notifications@github.com
wrote:

What do you mean by:

didn't shim any of the old locations

Are you talking code locations or URL paths?


Reply to this email directly or view it on GitHub
#443 (comment).

Contributor

bollwyvl commented Apr 21, 2015

Code locations! The URL scheme is unchanged! The only thing that should be
any different from outside is that places where something always said
"Github" might say "Gist" instead, as I tried to make the templates more
provider agnostic.

On Tue, Apr 21, 2015 at 12:03 PM Kyle Kelley notifications@github.com
wrote:

What do you mean by:

didn't shim any of the old locations

Are you talking code locations or URL paths?


Reply to this email directly or view it on GitHub
#443 (comment).

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 22, 2015

Member

Ok, had enough fun here: ready to review, but certainly open to any other changes. I still don't know how someone coming along would create and set custom configuration values for a custom provider except via environment variables!

I guess we could use IPython traitlets, the way nbconvert configuration does it, but that something for post 4.x split.

I must say that I hadn't looked at the code of nbviewer in a looooong time, but I'm really happy to see things going this way. This is really awesome !

Once this is merge I would suggest to write a blog post about nbviewer on blog.Jupyter.org if you like.

Member

Carreau commented Apr 22, 2015

Ok, had enough fun here: ready to review, but certainly open to any other changes. I still don't know how someone coming along would create and set custom configuration values for a custom provider except via environment variables!

I guess we could use IPython traitlets, the way nbconvert configuration does it, but that something for post 4.x split.

I must say that I hadn't looked at the code of nbviewer in a looooong time, but I'm really happy to see things going this way. This is really awesome !

Once this is merge I would suggest to write a blog post about nbviewer on blog.Jupyter.org if you like.

@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 22, 2015

Contributor

I guess we could use IPython traitlets, the way nbconvert configuration does it, but that something for post 4.x split.

Yeah: I was wondering about that. I was holding off buying into any more of the APIs while TBS™️ was going on. Since we already use traitlets for configuring nbconvert, it might be the path forward. My first look into that space suggested I should use BaseIPythonApp, but I wasn't sure how much baggage that would bring along. As it is, we don't really have a wrapper app around tornado, so the handlers basically have everything attached directly to them... having a Configurable (or whatever) might clean a lot of that behavior up!

The other thing that keeps bothering me is that this really looks like a setuptools entry_points problem... working on Allura, I really got used to writing extensibility that way: I am a fan of if it's installed, it should be used.

I recall a talk by Doug Hellman where he slams rolling your own plugin mechanism (which I have basically done, here), which I still think is probably true, but I also recall some animosity towards setuptools in the IPython ecosystem. If that's not the case, I'd really prefer to do it that way, though it would likely mean having an actual NBViewerPlugin class vs. just a python package as it is now... that would certainly help the documentation story!

Once this is merge I would suggest to write a blog post about nbviewer on blog.Jupyter.org if you like.

Sounds interesting! It would be great to have some numbers from the analytics to present in there:

  • a pretty graph of the data since we started keeping it
  • most popular notebooks
    ...and obviously would need some support from @rgbkrk to talk about the hosting angle!
Contributor

bollwyvl commented Apr 22, 2015

I guess we could use IPython traitlets, the way nbconvert configuration does it, but that something for post 4.x split.

Yeah: I was wondering about that. I was holding off buying into any more of the APIs while TBS™️ was going on. Since we already use traitlets for configuring nbconvert, it might be the path forward. My first look into that space suggested I should use BaseIPythonApp, but I wasn't sure how much baggage that would bring along. As it is, we don't really have a wrapper app around tornado, so the handlers basically have everything attached directly to them... having a Configurable (or whatever) might clean a lot of that behavior up!

The other thing that keeps bothering me is that this really looks like a setuptools entry_points problem... working on Allura, I really got used to writing extensibility that way: I am a fan of if it's installed, it should be used.

I recall a talk by Doug Hellman where he slams rolling your own plugin mechanism (which I have basically done, here), which I still think is probably true, but I also recall some animosity towards setuptools in the IPython ecosystem. If that's not the case, I'd really prefer to do it that way, though it would likely mean having an actual NBViewerPlugin class vs. just a python package as it is now... that would certainly help the documentation story!

Once this is merge I would suggest to write a blog post about nbviewer on blog.Jupyter.org if you like.

Sounds interesting! It would be great to have some numbers from the analytics to present in there:

  • a pretty graph of the data since we started keeping it
  • most popular notebooks
    ...and obviously would need some support from @rgbkrk to talk about the hosting angle!
@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 26, 2015

Member

Do you want to wait on exposing alternate providers in traitlets for a later PR?

Member

rgbkrk commented Apr 26, 2015

Do you want to wait on exposing alternate providers in traitlets for a later PR?

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk
Member

rgbkrk commented Apr 28, 2015

:shipit:

rgbkrk added a commit that referenced this pull request Apr 28, 2015

@rgbkrk rgbkrk merged commit d2b8807 into jupyter:master Apr 28, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bollwyvl

This comment has been minimized.

Show comment
Hide comment
@bollwyvl

bollwyvl Apr 29, 2015

Contributor

Great. Tried to never leave it broken... But i totally have a draft in my inbox for how i was trying out setuptools entry_points. I am much happier with it, and will open a separate pr! I started a straw man gitlab integration (we stated using gl at work), and the API was very smooth... Up until the client.

Contributor

bollwyvl commented Apr 29, 2015

Great. Tried to never leave it broken... But i totally have a draft in my inbox for how i was trying out setuptools entry_points. I am much happier with it, and will open a separate pr! I started a straw man gitlab integration (we stated using gl at work), and the API was very smooth... Up until the client.

@rgbkrk

This comment has been minimized.

Show comment
Hide comment
@rgbkrk

rgbkrk Apr 29, 2015

Member

Welp, we'll see what happens when it deploys. 😉

Member

rgbkrk commented Apr 29, 2015

Welp, we'll see what happens when it deploys. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment