Move away from let-bindings

[tarsius]

When let-bindings were only used to set use another github instance, then it made sense. That was a use-case I knew I had to support but I figured it would be used very little, so I did not want to "pollute" the function signatures. Back then we already also used it to signal ghub--request to unpaginate a resource, and that was a bit strange. I think I did it because I did it for the "instance" also, and maybe I felt that was appropriate because url.el itself makes heavy use of let-bindings.

But then more and more new "variables to be let-bound" got added, and now I find this really weird and old-fashioned.

I was experimenting with adding function arguments as an alternative method to request non-default behavior. But that got out of hand quickly - lots of documentation and accidental complexity.

I am now considering to give up let-bindings altogether and to use function arguments exclusively. This would break backward compatibility, but I think now is the time to make such changes.

/cc @vermiculus

[vermiculus]

I'd be ok with this change provided we can coordinate – probably in develop branch or similar – so things aren't terribly broken for any substantial period of time for magithub users 😄 GitHub's PR review feature may come in handy here.

This begs the question: how would existing 'output' variables be reimplemented (i.e., ghub-response-headers)?

[tarsius]

Yes, I will do this on a feature branch.

Also I think I should probably release v2 soon and then v3 when merging that branch into master.

[vermiculus]

That sounds good. Can you make sure to ping me again whenever you push that branch? :-)

[tarsius]

The pu branch implements both #23 and #25. Both features are not complete yet and subject to change, but I am already using this branch in production and so far it is holding together well.

[vermiculus]

Thanks! I've been testing it out, but I'm running into an issue with the following expression:

(ghub-get "/authorizations" nil nil nil nil nil nil nil 'basic)

Here's my backtrace

Debugger entered--Lisp error: (wrong-type-argument stringp [cl-struct-url "http" nil nil "api.github.com" nil "/authorizations" nil nil t nil t])
  string-match("\\`https?://\\([^/]+\\)" [cl-struct-url "http" nil nil "api.github.com" nil "/authorizations" nil nil t nil t])
  (if (string-match "\\`https?://\\([^/]+\\)" url) (match-string 1 url) (signal (quote ghub-auth-error) (list (format "Invalid url %s" url))))
  (progn (if (string-match "\\`https?://\\([^/]+\\)" url) (match-string 1 url) (signal (quote ghub-auth-error) (list (format "Invalid url %s" url)))))
  (unwind-protect (progn (if (string-match "\\`https?://\\([^/]+\\)" url) (match-string 1 url) (signal (quote ghub-auth-error) (list (format "Invalid url %s" url))))) (set-match-data save-match-data-internal (quote evaporate)))
  (let ((save-match-data-internal (match-data))) (unwind-protect (progn (if (string-match "\\`https?://\\([^/]+\\)" url) (match-string 1 url) (signal (quote ghub-auth-error) (list (format "Invalid url %s" url))))) (set-match-data save-match-data-internal (quote evaporate))))
  ghub--hostname([cl-struct-url "http" nil nil "api.github.com" nil "/authorizations" nil nil t nil t])
  (or hostname (ghub--hostname url))
  (format "github.%s.user" (or hostname (ghub--hostname url)))
  (if (string-prefix-p "https://api.github.com" url) "github.user" (format "github.%s.user" (or hostname (ghub--hostname url))))
  (let ((var (if (string-prefix-p "https://api.github.com" url) "github.user" (format "github.%s.user" (or hostname (ghub--hostname url)))))) (condition-case nil (car (process-lines "git" "config" var)) (error (signal (quote ghub-auth-error) (list (format "%s is undefined" var))))))
  ghub--username([cl-struct-url "http" nil nil "api.github.com" nil "/authorizations" nil nil t nil t])
  (aset v 2 (ghub--username url))
  (let* ((v url)) (aset v 2 (ghub--username url)))
  (progn nil (or (and (memq (aref url 0) cl-struct-url-tags)) (signal (quote wrong-type-argument) (list (quote url) url))) (let* ((v url)) (aset v 2 (ghub--username url))))
  (let ((url (url-generic-parse-url url))) (progn nil (or (and (memq (aref url 0) cl-struct-url-tags)) (signal (quote wrong-type-argument) (list (quote url) url))) (let* ((v url)) (aset v 2 (ghub--username url)))) (url-basic-auth url t))
  ghub--basic-auth("http://api.github.com/authorizations")
  (if (eq auth (quote basic)) (ghub--basic-auth url) (concat "token " (if (stringp auth) auth (ghub--token url username (and (symbolp auth) auth)))))
  (encode-coding-string (if (eq auth (quote basic)) (ghub--basic-auth url) (concat "token " (if (stringp auth) auth (ghub--token url username (and (symbolp auth) auth))))) (quote utf-8))
  ghub--auth("http://api.github.com/authorizations" nil basic)

The url that's getting passed to ghub--hostname is not in fact a string, but a full URL CL structure. I think the code is being a little confusing calling url-the-string and url-the-object the same thing in the same function.

[vermiculus]

Also, a little bit of code review, is this correct?

(defun ghub--auth (url username auth)
  (encode-coding-string
   (if (eq auth 'basic)
       (ghub--basic-auth url)
     (concat "token "
             (if (stringp auth)
                 auth
               (ghub--token url username (and (symbolp auth) auth)))))  ;<--
   'utf-8))

The third argument of ghub--token is written as package.

[tarsius]

Also, a little bit of code review, is this correct?

Yes. If ghub-request's auth is a symbol (except none and basic), then that means "use the token for that package". A string is treated as a literal token.

[tarsius]

I think the code is being a little confusing calling url-the-string and url-the-object the same thing in the same function.

It was fine until the only use of the url-the-string was to convert it into url-the-object. Anyway that is fixed now. Thanks!

[vermiculus]

Yes. If ghub-request's auth is a symbol (except none and basic), then that means "use the token for that package". A string is treated as a literal token.

So is t no longer an acceptable value here?

[tarsius]

Yes that is correct. Because the authentication method is now specified using an argument, which we don't want to specify every time we want to use the default method, that method is no longer identified by t (aka "hey there, do use the default authentication method") but by nil (aka "just do what you do by default"). What was previously nil is now none.

[vermiculus]

Alright, token creation works pretty well now (as in I can create a token Emacs package magithub), but I'm failing on (funcall save) in ghub-create-token:

Debugger entered--Lisp error: (invalid-function (("~/.authinfo <new token, now deleted>" . "ran") ("~/.netrc" :mtime (21632 42622 0 0) :secret (lambda (&rest --cl-rest--) (apply (quote #[(G0) "\301\302\303\304�J\"\"\207" [G0 apply string mapcar 1-] 5]) (quote --v--) --cl-rest--))) ("~/.authinfo" :mtime (22847 57650 0 0) :secret (lambda (&rest --cl-rest--) (apply (quote #[(G0) "\301\302\303\304�J\"\"\207" [G0 apply string mapcar 1-] 5]) (quote --v--) --cl-rest--)))))
  (("~/.authinfo <new token>" . "ran") ("~/.netrc" :mtime (21632 42622 0 0) :secret (lambda (&rest --cl-rest--) (apply (quote #[(G0) "\301\302\303\304�J\"\"\207" [G0 apply string mapcar 1-] 5]) (quote --v--) --cl-rest--))) ("~/.authinfo" :mtime (22847 57650 0 0) :secret (lambda (&rest --cl-rest--) (apply (quote #[(G0) "\301\302\303\304�J\"\"\207" [G0 apply string mapcar 1-] 5]) (quote --v--) --cl-rest--))))()
  funcall((("~/.authinfo <new token>" . "ran") ("~/.netrc" :mtime (21632 42622 0 0) :secret (lambda (&rest --cl-rest--) (apply (quote #[(G0) "\301\302\303\304�J\"\"\207" [G0 apply string mapcar 1-] 5]) (quote --v--) --cl-rest--))) ("~/.authinfo" :mtime (22847 57650 0 0) :secret (lambda (&rest --cl-rest--) (apply (quote #[(G0) "\301\302\303\304�J\"\"\207" [G0 apply string mapcar 1-] 5]) (quote --v--) --cl-rest--)))))
  (progn (funcall save) (auth-source-forget (list :host host :user user)) token)
  (let* ((--cl-rest-- (ghub--auth-source-get (list :save-function :secret) :create t :host host :user user :secret (cdr (assq (quote token) (ghub-post "/authorizations" nil (list ... ...) nil nil nil nil username (quote none)))))) (save (if (= (length --cl-rest--) 2) (car-safe (prog1 --cl-rest-- (setq --cl-rest-- (cdr --cl-rest--)))) (signal (quote wrong-number-of-arguments) (list nil (length --cl-rest--))))) (token (car --cl-rest--))) (progn (funcall save) (auth-source-forget (list :host host :user user)) token))
  (let ((host (ghub--hostname url)) (user (ghub--ident package username))) (let* ((--cl-rest-- (ghub--auth-source-get (list :save-function :secret) :create t :host host :user user :secret (cdr (assq (quote token) (ghub-post "/authorizations" nil ... nil nil nil nil username ...))))) (save (if (= (length --cl-rest--) 2) (car-safe (prog1 --cl-rest-- (setq --cl-rest-- ...))) (signal (quote wrong-number-of-arguments) (list nil (length --cl-rest--))))) (token (car --cl-rest--))) (progn (funcall save) (auth-source-forget (list :host host :user user)) token)))
  ghub-create-token("https://api.github.com/user" magithub nil ("repo" "user" "notifications" "read:org"))
  ;; blah blah blah
  ghub-request("GET" "/user" nil nil nil nil nil nil nil magithub nil)

What was intended here?

[tarsius]

Seems I did not re-test everything after making changes...

[vermiculus]

I can confirm your recent changes have fixed the issue, though see my comment 😉

Any idea on when pu will be pulled into master? I'm wondering if I should pull my supporting/incompatible changes out of my next ghub+ push or just wait a little longer.

[tarsius]

Any idea on when pu will be pulled into master?

Any time now 😉 I am currently finishing up old stuff that is mostly ready but hasn't hit master yet (mostly, but not only, Magit's master). This is one of them. I want to make a Magit release at the end of next week and this has to be done by then.