We should remove the old notebook from Sage. #121

Merged
merged 2 commits into from Mar 22, 2013

4 participants

@jhpalmieri

The changes here are necessary for http://trac.sagemath.org/sage_trac/ticket/11409. These were mainly assembled by Jeroen Demeyer, and John Palmieri made minor contributions.

@ppurka ppurka commented on an outdated diff Dec 7, 2012
sagenb/misc/misc.py
@@ -85,14 +86,16 @@ def print_open_msg(address, port, secure=False, path=""):
print '*'*n
-def find_next_available_port(interface, start, max_tries=100, verbose=False):
+def find_next_available_port(host, start, max_tries=100, verbose=False):
@ppurka
Sage Mathematical Software System member
ppurka added a line comment Dec 7, 2012

Shouldn't this argument remain as "host"? It corresponds to the "interface" argument that can be passed on to the notebook.

(I remember there was some discussion about the interface argument, but I can't remember where. So, ignore this comment if this change is because of that discussion.)

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

I'm not sure what you mean by 'remain as "host"': it's being changed from "interface" to "host". I don't feel strongly about it -- I took it from Jeroen's patch at trac #11409 -- but since the argument is supposed to be something like '127.0.0.1', "host" seems like a better name.

@kini
Sage Mathematical Software System member

FWIW, a single host may have multiple network interfaces, and IP addresses (such as 127.0.0.1) are associated with a network interface, not with a host.

@ppurka
Sage Mathematical Software System member

Oops sorry. That was a typo.What I meant to ask was whether it should "remain as 'interface'". Thanks for the pointer to #11409. I couldn't remember where I had seen the discussion on the change of interface to host.

@jhpalmieri

Okay, I think I've changed it back now.

@rmartinjak

how about "listen" or "listen_address", as it is often used for such an option? "interface" could be mistaken for the physical interface, e.g. eth0

@ppurka
Sage Mathematical Software System member

I need to check just jmol (don't have a working java here). Otherwise, everything else is working with #11409 + this patch.

@jhpalmieri

By the way, I think that although this is required for #11409, I think that this does not depend on #11409. It would be good to check.

@ppurka
Sage Mathematical Software System member

This needs #11409. Doesn't work without that.

Also, I checked jmol on firefox-17. It works. So, positive review from my side.

@kini: up to you now to merge this :)

@ppurka
Sage Mathematical Software System member

Just an update. I, of course, didn't use the sagenb patch from #11409 when I applied the patches in that ticket.

@ppurka
Sage Mathematical Software System member

Another update: #13794, by itself, works just fine without this pull.

@jhpalmieri

What does this need from #11409? It seems to work fine for me without it. ("sage -n" doesn't work, but that's a separate issue.)

@ppurka
Sage Mathematical Software System member

Right now the status is this (when I say "it works", I mean "sage -n" works):

  1. #13794 works with the nb that comes with 5.5rc0.
  2. This pull request by itself doesn't work.
  3. #13794 + #11409 + this pull request works on 5.5rc0 (with sagenb git).
@jhpalmieri

Right. Let's leave "sage -n" aside. With only this pull request, does the notebook function? If so, this can be merged independently of #13794 and #11409.

@ppurka
Sage Mathematical Software System member

Ok. I can confirm that the notebook works with this pull request and without #11409 or #13794.

But then the notebook in 5.5rc0 still works without #11409. It is possible to launch it from the command line by using sage -c 'notebook()' or by running sage first and then launching notebook().

@jhpalmieri

Good. The point is that merging this does not need to be carefully coordinated with either #13794 or #11409 (except that #11409 depends on this).

@ppurka
Sage Mathematical Software System member

Yes. I would prefer that #13794 goes in first. Then this pull request can go in independently of #11409 (it seems to work with #13794).

@ppurka
Sage Mathematical Software System member

What is the status of this ticket? Are there any other modifications to be done? It works fine on 5.7beta0. If there are no objections, we can merge this.

@kini
Sage Mathematical Software System member

I have no objection, at least. I guess I'll just merge it, then.

@kini kini merged commit 72b4a58 into sagemath:master Mar 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment