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

verb-set-var fix #51

Closed bigodel closed 1 year ago

bigodel commented 1 year ago

Currently, on interactive calls, verb-set-var always prompts on a value for the variable nil. That happens because it checks for the following (and (symbolp var) (symbol-name var)) and that always results in "nil", since (symbolp nil) returns true. I propose the following:

Using the mechanism from interactive to specify a list of arguments to be used on interactive calls, here prompting the user for them like it was done before. in any other case, rely on the old behaviour.

Notice that for this new version, the variable var is required.

federicotdn commented 1 year ago

Hi! Thanks for opening this PR. I decided to go with simply adding a check for var inside the and so that when nil is used, the and value is not used. This way we also maintain the usage of symbols as keys for verb--vars, which I prefer over strings. See here: https://github.com/federicotdn/verb/commit/ba215b70fb09f35656d0791599be2d5abc3113d8 Edit: sorry, misread part of your code. Your code still uses symbols as keys, however the shorter fix is preferable to maintain the function signature.

bigodel commented 1 year ago

that's cool, it solves it. only thing i'd point out is that with this change there is no need to keep using assoc-string on line 1082 to get the variable's value, assoc should do it.