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

Incorrect Return Value in readme #105

Open
nekufa opened this issue Oct 10, 2016 · 12 comments
Open

Incorrect Return Value in readme #105

nekufa opened this issue Oct 10, 2016 · 12 comments
Milestone

Comments

@nekufa
Copy link

nekufa commented Oct 10, 2016

Hello, i found that readme is incorrect.

There are a lot of comments like bool return "False and raises Exception on error".
It's impossible to return value and raise the exception.

What will happen? False will be returned or exception thrown?

@bigbes
Copy link
Contributor

bigbes commented Oct 11, 2016

C API algorithm:

  1. Exception variable set
  2. return from function with RETURN_FALSE
    tell me - what will happen?

@rybakit
Copy link
Collaborator

rybakit commented Oct 11, 2016

@bigbes If I get it right, the question is why a method returns a boolean value. What is the point of returning bool true/false if it will always be true on success and an exception on failure?

@bigbes
Copy link
Contributor

bigbes commented Oct 11, 2016

@rybakit Is that a problem? Seems like something, that won't question anyone.

@rybakit
Copy link
Collaborator

rybakit commented Oct 11, 2016

@bigbes Imho, the problem that api gives a wrong impression that a user can just check the result of a method call, like:

if (!$t->ping()) {
   // this block will never be reached
}

@bigbes
Copy link
Contributor

bigbes commented Oct 11, 2016

It's not something, that bothers you before?

@rybakit
Copy link
Collaborator

rybakit commented Oct 11, 2016

@bigbes See this comment: #44 (comment)

@bigbes
Copy link
Contributor

bigbes commented Oct 11, 2016

Can you provide PR that will fix README.md?

@rybakit
Copy link
Collaborator

rybakit commented Oct 11, 2016

Of course, I can update README.md if you are OK to tweak the implementation.

@bigbes
Copy link
Contributor

bigbes commented Oct 11, 2016

What is the point of returning bool true/false if it will always be true on success and an exception on failure?

Implementation is OK, as I've understand, README gives wrong impression. Not?

@rybakit
Copy link
Collaborator

rybakit commented Oct 11, 2016

Hm, so you propose to modify the documentation to not reflect the real behavior?

@bigbes
Copy link
Contributor

bigbes commented Oct 11, 2016

What is the point of returning bool true/false if it will always be true on success and an exception on failure?

You have told, that this is the real behaviour, not?

@rybakit
Copy link
Collaborator

rybakit commented Oct 11, 2016

I didn't get your question, sorry. My point (although I'm not alone, as I'm not the one who created this issue /cc @nekufa) that mixing return booleans and exceptions is bad and I've already described and gave an example of why I think the api has the flaw. So, to fix that both the implementation and documentation should be updated to always return void and throw an exception in a case of an error. It doesn't have sense to me to only update the documentation and keep the current behavior. There is nothing much I can add to this discussion, but I hope I was clear enough ;)

P.S. I would really appreciate if you can provide me with the links to the docs or libraries which implement this "bool/exceptions" design, I have never seen this before. Maybe this will help me to understand your implementation.

@Totktonada Totktonada added this to the wishlist milestone Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants