plz.el icon indicating copy to clipboard operation
plz.el copied to clipboard

Sync requests do not ignore `:else`

Open josephmturner opened this issue 2 years ago • 8 comments

The plz docstring says "ELSE is an optional callback function... For synchronous requests, this argument is ignored."

It appears that sync requests do not ignore :else:

(plz 'get "https://fake.fake"
  :else #'ignore)

returns nil instead of signalling an error.

josephmturner avatar May 20 '23 05:05 josephmturner

Thanks. I should add a test for this.

alphapapa avatar May 23 '23 15:05 alphapapa

Looking at this again, I'm not sure what the best API is. Would it be okay for synchronous requests to accept the ELSE argument for a function to call if the request fails? The choices look something like this:

(unless (ignore-errors
          (plz 'get "https://fake.fake"))
  ...)
(or (ignore-errors
      (plz 'get "https://fake.fake"))
    ...)

(unless (plz 'get "https://fake.fake"
          :else #'ignore)
  ...)
(or (plz 'get "https://fake.fake"
      :else #'ignore)
    ...)

And I'm not sure which is best. Maybe it would be okay to let users pass an ELSE, or maybe we should disallow it for sync requests. What do you think? (Probably we already discussed this and I forgot what we decided.)

alphapapa avatar Jun 14 '23 13:06 alphapapa

FWIW, there is at least some code in the wild that uses :else #'ignore with sync requests, and would break if changed, e.g. https://github.com/jinnovation/kele.el/blob/15e841fb7bbc08545534e466ce831d6e80fd8901/kele.el#L459

@jinnovation Do you have any thoughts about this?

@Fuco1 I see that org-node-graph also uses plz (although I don't know if its code would be affected by changing this). What do you think?

@akirak I'd like to know what you think, too, if you would.

alphapapa avatar Jun 15 '23 06:06 alphapapa

For synchronous requests I would most prefer it to throw an error I can handle with condition-case or ignore. I like that I can just let-bind the result and not worry about it, it makes the code very clean. In case of error, most likely I can't continue anyway so an error handler seems like a good place to jump to.

If an :else is there, plz can somehow handle the error internally and use that form/function as the error handler? But maybe I would deprecate such use.

Fuco1 avatar Jun 15 '23 09:06 Fuco1

I prefer the documented behavior over the current behavior, but I don't feel strongly about it.

josephmturner avatar Jun 17 '23 19:06 josephmturner

I'd like to know what you think, too, if you would.

As you might know, the uniform API to chain asynchronous operations is promises.

async/await is a syntactic pattern that incorporates promises into synchronous computation. I never recommend doing things like below, because it doesn't handle errors and can lead to an unexpected behavior:

(unless (ignore-errors
          (plz 'get "https://fake.fake"))
  ...)
(or (ignore-errors
      (plz 'get "https://fake.fake"))
    ...)

I personally find promise.el the easiest to use among several promise libraries. Its promise-chain form is what one should use here.

I've only used plz.el for simple use cases, so I didn't pay much attention to the :else argument. My usage has been only synchronous and has had no error handling (which would be bad in mission critical software but my Emacs packages are nothing like that). For complex things, it would be nice if there were support for ease of integration with promise.el.

akirak avatar Jun 18 '23 01:06 akirak

@akirak FYI, you might be interested in this (though it uses aio rather than promise):

https://github.com/alphapapa/plz.el/blob/wip/aio/aio-plz.el https://github.com/alphapapa/plz.el/blob/wip/aio/tests/test-aio-plz.el

alphapapa avatar Jul 03 '23 14:07 alphapapa

Since this is not a new bug or issue, and I'm still not sure how I want to handle it, I'm deferring it to v0.8 so that v0.7 can be tagged now.

alphapapa avatar Jul 09 '23 19:07 alphapapa