feat: Allow specifying protocol version in request specs #75
Conversation
There was a problem hiding this comment.
Hi, before I review the code, would you mind applying formatting?
Mostly, calling indent-region on the code, and making sure that parenthesis are never on their own in a line, e.g.:
(defun verb--protocol-as-curl-option (protocol)
"Return the corresponding curl option for
a given http protocol."
(unless (verb--http-protocol-p protocol)
(user-error "Please pass a valid http protocol"))
(let ((protocol-maps (list (cons "HTTP/0.9" "--http0.9")
(cons "HTTP/1.0" "--http1.0")
(cons "HTTP/1.1" "--http1.1")
(cons "HTTP/2" "--http2")
(cons "HTTP/3" "--http3"))))
(alist-get protocol protocol-maps nil nil 'equal)))instead of:
(defun verb--protocol-as-curl-option (protocol)
"Return the corresponding curl option for
a given http protocol."
(unless (verb--http-protocol-p protocol)
(user-error "Please pass a valid http protocol")
)
(let (
(protocol-maps (list (cons "HTTP/0.9" "--http0.9")
(cons "HTTP/1.0" "--http1.0")
(cons "HTTP/1.1" "--http1.1")
(cons "HTTP/2" "--http2")
(cons "HTTP/3" "--http3")
))
)
(alist-get protocol protocol-maps nil nil 'equal))
)
Plus, please remove all empty lines within functions, and between functions, please only leave one line. Additionally, we should replace all tabs with spaces.
|
Hey, I fixed the spacing and the parentheses. I also ran make check. However make check keeps giving me errors that would be resolved by compat. I am using seq-positions, string-split and flatten-tree. All of those functions are covered by compat, which it looks like you install for make check. On top of this, you require seq. Not sure how to resolve this issue. |
|
So bumping the emacs version this package require and adding compat to the header resolved my issue - Package-Requires: ((emacs "29.1") (compat "30.0.0.0")). This was your previous header: Package-Requires: ((emacs "26.3")). According to the compat manual, compat needs to be apart of the Package-Requires list and seq-positions was added in emacs 29.1. If you don't want to bump the emacs version this package requires, I could also just copy the seq-positions function and add it to verb and then set this as the header - Package-Requires: ((emacs "26.3") (compat "30.0.0.0"). |
|
Hi, currently Verb targets Emacs 26 or newer (meaning that it can only use functions that were available up to Emacs 26). |
|
In that case, I will just do Package-Requires:((emacs 26.3) (compat
30.0.0.0)) and then add seq-positions to verb.el. Compat supports the rest
of seq.el so I’m sure it will be added in their next update. When that
happens, seq-positions can be removed.
|
Added a new constant, a list of valid http-protocols. Also added a function that checks if a value is a member of this list along with a test for this function.
These tests are for the new protocol field that will be added to verb-request-spec.
This step is necessary as the plan is to turn the METHOD+URL section into METHOD+URL+PROTOCOL, as suggested in the original issue: "get http://example.com http/1.1". More code will obviously have to be added in order match protocols so in this step I tried to simplify the METHOD+URL section and extract out functions that could also be used to match protocols. Tests were are added for all three functions that I extracted out.
METHOD+URL section in verb-request-spec-from-string The refactor started in my last commit is now finished. It would have been very difficult to add protocol to the original METHOD+URL section as it already contained nested conditional logic. Instead, I wrote a verb--single-line-method-url-protocol, a function that splits the method+url+protocol line in to parts. Method and protocol are then validated using validation functions that I added for this commit while the url is simply stored. If the url is on multiple lines, the lines are concatenated together and then if a protocol is also specified, it's stored. Once I tested this refactor, I added protocol to the verb-request-spec created at the the of verb-request-spec-from-string function.
fix: Added compat and seq-positions
|
Done |
|
Apologies for the delay in reviewing. Would it be possible to implement the functionality without adding the |
|
I think I am just going to close the pull request. I thought you would be ok with compat as you were already using it as a dev dependency. I could remove it and replace the functions that use it but that certainly wouldn't resolve the problems you have with the amount of code I added. The main issue was I had to work around the existing code you were using to get the METHOD+URL line while of course making sure my changes didn't break anything. One approach would have been add two more nested if's on top of another regex. I believe that would have made the code very hard to read and debug, while also making the METHOD+URL+PROTOCOL way longer than necessary. |
In reference to issue #65. Obviously url-retrieve does not support specifying a protocol, so this addition is really only useful for curl exports. If a specific http protocol is included in a request, the corresponding curl option is now added for the export.