Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync requests do not ignore :else #29

Open
josephmturner opened this issue May 20, 2023 · 8 comments
Open

Sync requests do not ignore :else #29

josephmturner opened this issue May 20, 2023 · 8 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@josephmturner
Copy link
Contributor

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.

@alphapapa alphapapa self-assigned this May 23, 2023
@alphapapa alphapapa added the bug Something isn't working label May 23, 2023
@alphapapa
Copy link
Owner

Thanks. I should add a test for this.

@alphapapa alphapapa added this to the 0.6 milestone May 23, 2023
@alphapapa
Copy link
Owner

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 alphapapa modified the milestones: 0.6, 0.7 Jun 14, 2023
@alphapapa
Copy link
Owner

alphapapa commented Jun 15, 2023

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.

@Fuco1
Copy link

Fuco1 commented Jun 15, 2023

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.

@josephmturner
Copy link
Contributor Author

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

@akirak
Copy link

akirak commented Jun 18, 2023

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.

@alphapapa
Copy link
Owner

@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
Copy link
Owner

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 alphapapa modified the milestones: 0.7, 0.8 Jul 9, 2023
@alphapapa alphapapa modified the milestones: 0.8, v0.9 Apr 26, 2024
@alphapapa alphapapa modified the milestones: v0.9, v0.10 Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants