make use of library request.el #21

Merged
merged 1 commit into from Jun 29, 2013

Conversation

Projects
None yet
1 participant
Contributor

tarsius commented Apr 6, 2013

You probably don't want to merge this as-is but I think this is a good first step. I have done only little testing but already use this for the Emacsmirror. So far things look good.

This doesn't really make proper use of request.el yet but even in the current form it is a big improvement because when using the curl backend I don't constantly get errors any more like I did with url.el.

  1. The gh-request- prefix is used for some new functions. But the names of some old and new functions should be reconsidered. Also did not change the library name.
  2. Handling of retries and paging should be handled in request. I don't think it supports that yet, so we have to keep implementing that in gh.
  3. Paging can very easily exceed max-lisp-eval-depth and max-specpdl-size.
  4. gh-url-response no longer supports callbacks. This was only used for pcache and simply calling pcache-put directly after making the request is good enough for that purpose.
Contributor

tarsius commented Apr 9, 2013

I just noticed that the callbacks are actually need in gist.el. I will look into that, and update the requestel branch.

Contributor

tarsius commented Jun 12, 2013

I have updated my patch to not remove callback support. There are also some other, though minor, differences. I tried splitting the change into multiple commits but failed to do so.

While I have used the old patch for two months without any problem I have not tested the new version much. While looking at request.el and deferred.el I decided to write my own client library for the github api. I will probably keep using gh for a while but it will be easier for me to start from scratch especially because I want to make use of deferred.

So I think my patch is good, but you should still inspect it closely. It doesn't really make full use of request.el but it's still a big win. url.el just sucks, since I started using curl all the issues I previously had just went away.

(The tests still need to be updated.)

Contributor

tarsius commented Jun 12, 2013

Could you please revoke my commit rights, I couldn't find a way to do that myself.

Because github doesn't remember old revisions of a pull request I past the first version like this:

(Actually that's not the original: I rebased it onto master at some point.)

commit 4b16c2398efc8de5f198d66facbe9c85f04a6ba9
Author: Jonas Bernoulli <jonas@bernoul.li>
Date:   Sat Apr 6 19:02:02 2013 +0200

    make use of library request.el

diff --git a/.travis.yml b/.travis.yml
index 0452913..0ef92ea 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -20,7 +20,7 @@ before_install:
     fi
   - pip install --use-mirrors virtualenv-emacs
   - virtualenv_install_emacs --with-emacs=`which "$EMACS"` --marmalade
-  - elpa-get eieio pcache logito mocker
+  - elpa-get eieio pcache logito mocker request
   - emacs --version
 script:
   make test
diff --git a/gh-api.el b/gh-api.el
index c66f450..04f15e1 100644
--- a/gh-api.el
+++ b/gh-api.el
@@ -123,17 +123,12 @@
 (defclass gh-api-response (gh-url-response)
   ())

-(defun gh-api-json-decode (repr)
-  (if (or (null repr) (string= repr ""))
-      'empty
-    (let ((json-array-type 'list))
-      (json-read-from-string repr))))
-
 (defun gh-api-json-encode (json)
   (json-encode-list json))

-(defmethod gh-url-response-set-data ((resp gh-api-response) data)
-  (call-next-method resp (gh-api-json-decode data)))
+(defun gh-url-form-encode (form)
+  (mapconcat (lambda (x) (format "%s=%s" (car x) (cdr x)))
+             form "&"))

 (defclass gh-api-paged-request (gh-api-request)
   ((default-response-cls :allocation :class :initform gh-api-paged-response)))
@@ -141,31 +136,21 @@
 (defclass gh-api-paged-response (gh-api-response)
   ())

-(defun gh-api-paging-links (links-header)
-  (when links-header
-    (let ((links nil)
-          (items (split-string links-header ", ")))
-      (loop for item in items
-            if (string-match
-                "^<\\(.*\\)>; rel=\"\\(.*\\)\""
-                item)
-            do
-            (push (cons (match-string 2 item)
-                        (match-string 1 item))
-                  links))
-      links)))
-
-(defmethod gh-url-response-set-data ((resp gh-api-paged-response) data)
-  (let* ((previous-data (oref resp :data))
-         (links (gh-api-paging-links (cdr (assoc "Link" (oref resp :headers)))))
-         (next (cdr (assoc "next" links))))
-    (call-next-method)
-    (oset resp :data (append previous-data (oref resp :data)))
+(defmethod gh-api-paging-links ((resp gh-api-paged-response))
+  (let ((links-header (cdr (assoc "Link" (oref resp :headers)))))
+    (when links-header
+      (loop for item in (split-string links-header ", ")
+            when (string-match "^<\\(.*\\)>; rel=\"\\(.*\\)\"" item)
+            collect (cons (match-string 2 item)
+                          (match-string 1 item))))))
+
+(defmethod gh-request-set-data ((resp gh-api-paged-response) data)
+  (oset resp :data (append (oref resp :data) data))
+  (let ((next (cdr (assoc "next" (gh-api-paging-links resp))))
+        (req (oref resp :-req)))
     (when next
-      (let ((req (oref resp :-req)))
-        (oset resp :data-received nil)
-        (oset req :url next)
-        (gh-url-run-request req resp)))))
+      (oset req :url next)
+      (gh-url-run-request req resp))))

 (defmethod gh-api-authenticated-request
   ((api gh-api) transformer method resource &optional data params)
@@ -203,15 +188,12 @@
                                               (gh-url-form-encode data))
                                          ""))))))
     (cond (has-value ;; got value from cache
-           (gh-api-response "cached" :data-received t :data value))
+           (gh-api-response "cached" :data value))
           (key ;; no value, but cache exists and method is safe
            (let ((resp (make-instance (oref req default-response-cls)
                                       :transform transformer)))
              (gh-url-run-request req resp)
-             (gh-url-add-response-callback
-              resp (list #'(lambda (value cache key)
-                             (pcache-put cache key value))
-                         cache key))
+             (pcache-put cache key value)
              resp))
           (cache ;; unsafe method, cache exists
            (pcache-invalidate cache key)
@@ -223,9 +205,6 @@
                                     (oref req default-response-cls)
                                     :transform transformer))))))

-(define-obsolete-function-alias 'gh-api-add-response-callback
-  'gh-url-add-response-callback "0.6.0")
-
 (provide 'gh-api)
 ;;; gh-api.el ends here

diff --git a/gh-pkg.el b/gh-pkg.el
index 3558a5f..eea5fdf 100644
--- a/gh-pkg.el
+++ b/gh-pkg.el
@@ -1,2 +1,2 @@
 (define-package "gh" "%VERSION%" "A GitHub library for Emacs"
-  '((eieio "1.3") (pcache "0.2.3") (logito "0.1")))
+  '((eieio "1.3") (pcache "0.2.3") (logito "0.1") (request "0.2.0alpha2")))
diff --git a/gh-url.el b/gh-url.el
index 82774ec..bf8fc52 100644
--- a/gh-url.el
+++ b/gh-url.el
@@ -32,7 +32,7 @@
 ;;;###autoload
 (require 'eieio)

-(require 'url-http)
+(require 'request)

 (defclass gh-url-request ()
   ((method :initarg :method :type string)
@@ -46,44 +46,12 @@
    (default-response-cls :allocation :class :initform gh-url-response)))

 (defclass gh-url-response ()
-  ((data-received :initarg :data-received :initform nil)
-   (data :initarg :data :initform nil)
+  ((data :initarg :data :initform nil)
    (headers :initarg :headers :initform nil)
    (http-status :initarg :http-status :initform nil)
-   (callbacks :initarg :callbacks :initform nil)
    (transform :initarg :transform :initform nil)
    (-req :initarg :-req :initform nil)))

-(defmethod gh-url-response-set-data ((resp gh-url-response) data)
-  (let ((transform (oref resp :transform)))
-    (oset resp :data
-          (if transform
-              (funcall transform data)
-            data))
-    (oset resp :data-received t)))
-
-(defmethod gh-url-response-run-callbacks ((resp gh-url-response))
-  (let ((copy-list (lambda (list)
-                     (if (consp list)
-                         (let ((res nil))
-                           (while (consp list) (push (pop list) res))
-                           (prog1 (nreverse res) (setcdr res list)))
-                       (car list)))))
-    (let ((data (oref resp :data)))
-      (dolist (cb (funcall copy-list (oref resp :callbacks)))
-        (if (or (functionp cb) (symbolp cb))
-            (funcall cb data)
-          (apply (car cb) data (cdr cb)))
-        (object-remove-from-list resp :callbacks cb))))
-  resp)
-
-(defmethod gh-url-add-response-callback ((resp gh-url-response) callback)
-  (object-add-to-list resp :callbacks callback t)
-  (if (oref resp :data-received)
-    (gh-url-response-run-callbacks resp)
-    resp))
-
-;;; code borrowed from nicferrier's web.el
 (defun gh-url-parse-headers (data)
   (let* ((headers nil)
          (header-lines (split-string data "\n"))
@@ -106,67 +74,50 @@
          (push (cons name value) headers)))
     headers))

-(defmethod gh-url-response-finalize ((resp gh-url-response))
-  (when (oref resp :data-received)
-    (gh-url-response-run-callbacks resp)))
-
-(defmethod gh-url-response-init ((resp gh-url-response)
-                                 buffer)
-  (declare (special url-http-end-of-headers))
-  (unwind-protect
-      (with-current-buffer buffer
-        (let ((headers (gh-url-parse-headers
-                        (buffer-substring
-                         (point-min) (1+ url-http-end-of-headers)))))
-          (oset resp :headers headers)
-          (oset resp :http-status (read (cdr (assoc 'status-code headers)))))
-        (goto-char (1+ url-http-end-of-headers))
-        (let ((raw (buffer-substring (point) (point-max))))
-          (gh-url-response-set-data resp raw)))
-    (kill-buffer buffer))
-  (gh-url-response-finalize resp)
+(defmethod gh-url-run-request ((req gh-url-request) &optional resp)
+  (unless resp
+    (setq resp (make-instance (oref req default-response-cls))))
+  (oset resp :-req req)
+  (request (oref req :url)
+      :sync    (not (oref req :async))
+      :type    (oref req :method)
+      :headers (oref req :headers)
+      :params  (oref req :query)
+      :data    (oref req :data)
+      :parser  (apply-partially 'gh-request-parse-response resp)
+      :success (apply-partially 'gh-request-handle-success resp)
+      :error   (apply-partially 'gh-request-handle-error resp))
   resp)

-(defun gh-url-set-response (status req-resp)
-  (set-buffer-multibyte t)
-  (destructuring-bind (req resp) req-resp
-    (condition-case err
-        (progn
-          (oset resp :-req req)
-          (gh-url-response-init resp (current-buffer)))
-      (error
-       (let ((num (oref req :num-retries)))
-         (if (or (null num) (zerop num))
-             (signal (car err) (cdr err))
-           (oset req :num-retries (1- num))
-           (gh-url-run-request req resp)))))))
-
-(defun gh-url-form-encode (form)
-  (mapconcat (lambda (x) (format "%s=%s" (car x) (cdr x)))
-             form "&"))
-
-(defun gh-url-params-encode (form)
-  (concat "?" (gh-url-form-encode form)))
+(defun* gh-request-parse-response (resp)
+  (let ((transformer (oref resp :transform)))
+    (if transformer
+   (let ((json-array-type 'list))
+     (funcall transformer (json-read)))
+      (buffer-string))))
+
+(defmethod gh-request-set-data ((resp gh-url-response) data)
+  (oset resp :data data))
+
+(defun* gh-request-handle-success (resp &key response data
+                   &allow-other-keys)
+  (oset resp :http-status (request-response-status-code response))
+  (let ((headers (gh-url-parse-headers
+         (request-response--raw-header response))))
+    (oset resp :headers headers)
+    ;; this might recursively make more requests
+    (gh-request-set-data resp data))
+  resp)

-(defmethod gh-url-run-request ((req gh-url-request) &optional resp)
-  (let ((url-request-method (oref req :method))
-        (url-request-data (oref req :data))
-        (url-request-extra-headers (oref req :headers))
-        (url (concat (oref req :url)
-                     (let ((params (oref req :query)))
-                       (if params
-                           (gh-url-params-encode params)
-                         "")))))
-    (if (oref req :async)
-        (let* ((resp (or resp (make-instance (oref req default-response-cls))))
-               (req-resp (list req resp)))
-          (url-retrieve url 'gh-url-set-response (list req-resp))
-          resp)
-      (let* ((resp (or resp (make-instance (oref req default-response-cls))))
-             (req-resp (list req resp)))
-        (with-current-buffer (url-retrieve-synchronously url)
-          (gh-url-set-response nil req-resp))
-        resp))))
+(defun* gh-request-handle-error (resp &key error-thrown
+                     &allow-other-keys)
+  (let* ((req (oref resp :-req))
+    (num (oref req :num-retries)))
+    (if (or (null num) (zerop num))
+   (signal (car error-thrown)
+       (cdr error-thrown))
+      (oset req :num-retries (1- num))
+      (gh-url-run-request req resp))))

 (provide 'gh-url)
 ;;; gh-url.el ends here

@tarsius tarsius merged commit a9457ae into sigma:master Jun 29, 2013

1 check failed

default The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment