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

CURLOPT_READFUNCTION documentation error #2175

Closed
ghost opened this Issue Dec 13, 2017 · 7 comments

Comments

Projects
None yet
1 participant
@ghost

ghost commented Dec 13, 2017

On this page

https://curl.haxx.se/libcurl/c/CURLOPT_READFUNCTION.html

  1. The Description section refers to parameter "nmemb" but the function prototype parameter is "nitems".

  2. The Return Value section is confusing as the return value (as described in the Description section) is supplied by the user.

@bagder bagder changed the title from Online Documentation Error to CURLOPT_READFUNCTION documentation error Dec 13, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Dec 13, 2017

Member

Thanks!

The nmemb is clearly a mistake and I'll fix.

The return code however is a bit complicated. When this option is set, curl_easy_setopt() is called and the return code section thus describes what that curl_easy_setopt() call returns. The callback's return code is explained the DESCRIPTION section. This is done like this for all man pages for options that describe callback functions. Any suggestions on how we could make that clearer or less confusing?

Member

bagder commented Dec 13, 2017

Thanks!

The nmemb is clearly a mistake and I'll fix.

The return code however is a bit complicated. When this option is set, curl_easy_setopt() is called and the return code section thus describes what that curl_easy_setopt() call returns. The callback's return code is explained the DESCRIPTION section. This is done like this for all man pages for options that describe callback functions. Any suggestions on how we could make that clearer or less confusing?

bagder added a commit that referenced this issue Dec 13, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 3, 2018

Member

So, no suggestion after three weeks and I personally think it is pretty clear...

Member

bagder commented Jan 3, 2018

So, no suggestion after three weeks and I personally think it is pretty clear...

@bagder bagder closed this Jan 3, 2018

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 3, 2018

ghost commented Jan 3, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 3, 2018

Member

I did not! Please send them again!

Member

bagder commented Jan 3, 2018

I did not! Please send them again!

@bagder bagder reopened this Jan 3, 2018

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 3, 2018

ghost commented Jan 3, 2018

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 4, 2018

Member

First: the argument is not called nmemb anymore, and yes the read/write callbacks are completely made up like fread/fwrite. A decision we made eons ago. Maybe not the most clever one we've done, but one we can't easily change now... Besides, the name of an argument in a man page is a very arbitrary thing to me and only serves as something to refer back to from the description so it's not a terribly big deal I think.

In regards to RETURN VALUE on the page: that's how we document all options to curl_easy_setopt (and in fact the other two *setopt functions too). It is actually not that common to have stand-alone man pages for separate options to functions to this is an area where we can't compare with a lot of other pre-existing documentation.

In my head, the documentation is for what happens when setopt is called using that option, so that's the return value. But I understand your point here. I just don't see an easy fix.

changed to explicitly state that the return value being referred to pertains to curl_easy_setopt, not the callback function.

I'd prefer all options pages to use the same style/language, and most options don't set any callback pointer.

Maybe we should have a separate CALLBACK header that describes exactly what the callback is supposed to do and then CALLBACK RETURN VALUE that describes what the callback can return and how those values are treated by libcurl? (Those could then be used for all options man pages that describe setting callbacks, some 10-12 of them or so.)

Member

bagder commented Jan 4, 2018

First: the argument is not called nmemb anymore, and yes the read/write callbacks are completely made up like fread/fwrite. A decision we made eons ago. Maybe not the most clever one we've done, but one we can't easily change now... Besides, the name of an argument in a man page is a very arbitrary thing to me and only serves as something to refer back to from the description so it's not a terribly big deal I think.

In regards to RETURN VALUE on the page: that's how we document all options to curl_easy_setopt (and in fact the other two *setopt functions too). It is actually not that common to have stand-alone man pages for separate options to functions to this is an area where we can't compare with a lot of other pre-existing documentation.

In my head, the documentation is for what happens when setopt is called using that option, so that's the return value. But I understand your point here. I just don't see an easy fix.

changed to explicitly state that the return value being referred to pertains to curl_easy_setopt, not the callback function.

I'd prefer all options pages to use the same style/language, and most options don't set any callback pointer.

Maybe we should have a separate CALLBACK header that describes exactly what the callback is supposed to do and then CALLBACK RETURN VALUE that describes what the callback can return and how those values are treated by libcurl? (Those could then be used for all options man pages that describe setting callbacks, some 10-12 of them or so.)

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 28, 2018

Member

While we can (should?) improve the documentation and I'll welcome that, I don't think this is a bug so I'm now closing this issue.

Member

bagder commented Jan 28, 2018

While we can (should?) improve the documentation and I'll welcome that, I don't think this is a bug so I'm now closing this issue.

@bagder bagder closed this Jan 28, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.