When let-bindings were only used to set use another github instance, then it made sens

The pu branch implements both <a class="issue-link js

Also, a little bit of code review, is this correct? <div class="highlight highligh

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

Move away from let-bindings about ghub HOT 14 CLOSED

magit commented on July 17, 2024

Move away from let-bindings

from ghub.

Comments (14)

tarsius commented on July 17, 2024 1

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!

from ghub.

tarsius commented on July 17, 2024 1

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.

from ghub.

vermiculus commented on July 17, 2024

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)?

from ghub.

tarsius commented on July 17, 2024

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.

from ghub.

vermiculus commented on July 17, 2024

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

from ghub.

tarsius commented on July 17, 2024

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.

from ghub.

vermiculus commented on July 17, 2024

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.

from ghub.

vermiculus commented on July 17, 2024

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.

from ghub.

tarsius commented on July 17, 2024

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.

from ghub.

vermiculus commented on July 17, 2024

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?

from ghub.

vermiculus commented on July 17, 2024

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?

from ghub.

tarsius commented on July 17, 2024

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

from ghub.

vermiculus commented on July 17, 2024

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.

from ghub.

tarsius commented on July 17, 2024

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.

from ghub.

Move away from let-bindings about ghub HOT 14 CLOSED

Comments (14)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent