Pinging @jdfreder for review. We talked about this last week.

I just looked over the code, it's a small change and it looks good to me. I'll give it a quick test and we can wait to see what Travis thinks.

Pinging @ellisonbg so he is aware of this change in behavior.

Looks like a lot of the tests are failing because creating a widget now tries to open a comm, but TerminalInteractiveShell doesn't have comms. These seem more like failures in the tests than the code. The JS failures look like they might be linked to timeouts? I'm not sure. Can you look at these tests, @jdfreder (in your plentiful spare time :)

Awesome, I think this makes sense, just have to figure out the tests.

On Thu, Jul 17, 2014 at 12:58 PM, Jason Grout notifications@github.com
wrote:

Looks like a lot of the tests are failing because creating a widget now
tries to open a comm, but TerminalInteractiveShell doesn't have comms.
These seem more like failures in the tests than the code.

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

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Honestly, I don't think the problem was with the tests, but the comm itself. I've opened a PR against this branch, let me know what you think.

@jdfreder: it looks like your change wasn't enough to satisfy Travis.

This makes me sad, it passed on my local machine... One more PR coming your way, after I figure out what to do.

Could you take a look at jasongrout#3 ?

I merged your change. How do I restart the Travis tests?

Edit: never mind, it appears they already restarted

Any new commit (including a merge) will trigger a new test run on Travis. If you need to restart a test run on the same commit, one of us with push access to the repo has to do it.

@jdfreder: Travis is happy! Thanks!

I think this is ready to go.

👍 +1

@jasongrout the code here looks good, but I want to mention a couple things:

• When I implement JS side widget initiation, I'll have to be careful about trying to open a comm when the widget is constructed in the front-end, because that code will complicate the persistant widget stuff, where the widget needs to "reconnect" to itself in the backend if possible.
• I really think @minrk should take a quick look to the changes I made to the comm in here to make sure they make sense, before we merge this.

Since comm is now a trait of the widget, and the open call only does anything if comm is None, then I imagine that you can just pass the existing comm into the widget (IntSlider(comm=EXISTING_COMM)), and things will just work. It won't try to create a new comm, but will use the existing one.

Assigned to @minrk for review.

@jasongrout note this also needs a rebase.

this might actually be superseded by Sylvain's pull requests dealing with widget lifetimes. Next week I'll work on reviewing his widget lifetime commits, and nagging you guys to look at them too 😄.

Does he need to cherry-pick or reimplement the comm fix from here too?

I cleaned up #6194. It has all the commits of this PR and a couple more.

Great! Does that mean we should close this @jasongrout ?

Yes, please.