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

use pcall #5

Open
konsumer opened this issue Nov 7, 2021 · 5 comments
Open

use pcall #5

konsumer opened this issue Nov 7, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@konsumer
Copy link
Contributor

konsumer commented Nov 7, 2021

It might be helpful and make it a bit easier to use if this lib wrapped network calls in pcall and returned nil on fail, or some other method of telling caller it didn't work out than an exception. Currently, if there is an error you get stuff like this, unless you wrap it yourself:

Screenshot from 2021-11-06 17-53-21

It may be a bit more involved than just nil, like in this case I think the issue is a socket-reset (I am making too many requests in an update-loop.)

maybe if it returned response, error? In that case, you could ignore error if you wanted to, and it would work essentially the same.

@konsumer
Copy link
Contributor Author

konsumer commented Nov 7, 2021

In the above, using this actually fixes my immediate problem, in update:

pcall(function() REST.retrieve(dt) end)

So, I can just do that if this doesn't seem like a good idea for the lib.

@MrcSnm
Copy link
Owner

MrcSnm commented Nov 7, 2021

Yea, it doesn't seem to be a nice idea. But I do wish to fix that bug either, if I'm not null checking it is because I didn't stumble on that error, can you create a minimal example on that issue?

@MrcSnm MrcSnm added the bug Something isn't working label Nov 7, 2021
@konsumer
Copy link
Contributor Author

konsumer commented Nov 7, 2021

It's not a specific bug, as it will crash a love program on any error. For example, to recreate above, you'd need to run a web-server that resets the connection on too many requests (in my thing, I am using bettercap as the REST server.) I see the general problem of crashing a love-game on a bad request as the issue, not a specific type of failed request. As I said, I could just wrap all your stuff in pcall, but this lib could also do that to create a safer interface.

As a related-sidenote, I really recommend some simple examples in README. I had to go work this out from my code, which I made guessing from the source of your lib. I'd be happy to add them in a PR, even if I don't use this lib.

@MrcSnm
Copy link
Owner

MrcSnm commented Nov 7, 2021

So, the main problem is basically indexing a nil value. It could be null checked that part, unless there are even more bugs like that. If you wish to do a PR, you're welcome

@konsumer
Copy link
Contributor Author

konsumer commented Nov 7, 2021

So, the main problem is basically indexing a nil value.

That may be where we disagree. Like it is for this one thing, but other edge error cases would cause the same problem. Again, I am fine with wrapping your stuff with pcall to make it safer to my game.

In any programming language that has a kind of fatal error (like lua) you can take an "always throw" approach to errors (and handle all errors non-fatally, which in the case of lua is with pcall) or a "handle every error case that anyone ever found" approach. The second approach will sometimes over-complicate the interface, and some error-cases will be missed, unless the scope is very narrow. I personally am fine with either approach, but a mix makes things kind of confusing, and in my own stuff have a strong preference for the first approach.

Again, since I am not going to use this library, after all, it makes less sense for me to fix it, especially if it doesn't seem like a problem in the first place, to you. I will PR for the examples, though, as this seems like a simple thing, and I already wrote code for it.

konsumer added a commit to konsumer/REST-love that referenced this issue Nov 7, 2021
MrcSnm added a commit that referenced this issue Nov 12, 2021
Example usage (related to #5)
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

2 participants