Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Multi-Mechanize tests #355

Merged
merged 4 commits into from Mar 1, 2013

Conversation

Projects
None yet
2 participants
Contributor

ihanson commented Aug 4, 2012

No description provided.

Owner

jasongrout commented Aug 5, 2012

I really like your idea to make the session a context!

Contributor

ihanson commented Feb 27, 2013

Can this pull request be finally merged? It contains (among other things) a change to the API which is assumed in the the documentation in the wiki.

I just rebased it on top of the current master.

@jasongrout jasongrout commented on the diff Feb 28, 2013

handlers.py
@@ -100,19 +100,19 @@ def post(self):
logger.info("Starting session: %s"%timer)
kernel_id = yield gen.Task(km.new_session_async)
data = {"ws_url": ws_url, "kernel_id": kernel_id}
- if self.request.headers["Accept"] == "application/json":
+ if "frame" not in self.request.headers:
@jasongrout

jasongrout Feb 28, 2013

Owner

why is this change is needed? (that's an honest question---I'm really curious)

Contributor

ihanson commented Mar 1, 2013

At one point I changed the protocol from using a URL parameter to using an HTTP header to indicate whether to load a file using AJAX or iframes (used where CORS is needed for a POST request but not available). This turned out to problematic in several ways. For one, the sendRequest function is designed to be able to request files of any type, not just JSON, so checking for a requested type of application/json is not really appropriate. It also makes the API unnecessarily complicated; it may not be clear to someone using the API that this specific header is needed even when requesting plain JSON. I decided here to revert back to the old method of using URL parameters to specify the special cases of both JSONP and iframes.

Owner

jasongrout commented Mar 1, 2013

Thanks. I'm glad we have that info now in a comment here

@jasongrout jasongrout added a commit that referenced this pull request Mar 1, 2013

@jasongrout jasongrout Merge pull request #355 from ihanson/multimechanize
Multi-Mechanize tests
e40d6c4

@jasongrout jasongrout merged commit e40d6c4 into sagemath:master Mar 1, 2013

@ihanson ihanson deleted the unknown repository branch Mar 1, 2013

@jasongrout jasongrout referenced this pull request Mar 1, 2013

Closed

Too many open files #396

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