search

federicotdn / verb

Organize and send HTTP requests from Emacs
https://melpa.org/#/verb
GNU General Public License v3.0
545 stars 20 forks source link

feat: Allow specifying protocol version in request specs #75

Closed vHugoObject closed 2 months ago

vHugoObject commented 2 months ago

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.

vHugoObject commented 2 months ago

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.

vHugoObject commented 2 months ago

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").

federicotdn commented 2 months ago

Hi, currently Verb targets Emacs 26 or newer (meaning that it can only use functions that were available up to Emacs 26). The check recipe however is ran only under Emacs 29, in the CI. Tests are ran for both 26 and 29. Locally, it should be enough to just run the checks and tests using Emacs 29. I've enabled the CI in this PR as well, which should help in debugging.

vHugoObject commented 2 months ago

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.

vHugoObject commented 2 months ago

Done

federicotdn commented 2 months ago

Apologies for the delay in reviewing. Would it be possible to implement the functionality without adding the compat dependency? I do not want to add any dependencies to this project. To be honest I though this change would have required much less code, maybe I underestimated the problem.

vHugoObject commented 2 months ago

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.