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

"integer representing the status" is insufficient documentation #694

Open
TELunus opened this issue Mar 14, 2018 · 8 comments
Open

"integer representing the status" is insufficient documentation #694

TELunus opened this issue Mar 14, 2018 · 8 comments

Comments

@TELunus
Copy link

TELunus commented Mar 14, 2018

The functions in the category Diagnostics and Repair say they return

An integer representing the status.

This documentation is insufficient to use the result. It seems likely to me that anything other than a 0 is an error. However the parse functions return a 2 for an error, but 1 and 0 are both no error (1 indicates a warning, but that's still not an error). In theory it could even be reversed: 0 is an error, non-zero tells how many good elements there are, for example. Without knowing how the integer represents the status, it's useless.

@geoffmcl
Copy link
Contributor

@TELunus thank you for your issue...

As you point out, libtidy, and thus the console app, tidy "universally adhere to the following convention" - see libtidy_03 -

  1. 0 == Success, good to go.
  2. 1 == Warnings, but no errors.
  3. 2 == Errors (and maybe warnings).
  4. <0 == Severe error. Usually the value equals -errno. See errno.h.

So I am afraid I do not understand your it's useless! Please explain more...

See the very simple sample program on how this can be used...

If you are using libtidy in your own app, then the API includes setting callback functions to filter the message output - see I/O and Messages - scroll down to the TidyMessageCallback API...

Does this answer your question? Thanks...

@geoffmcl geoffmcl added this to the 5.7 milestone Mar 15, 2018
@TELunus
Copy link
Author

TELunus commented Mar 16, 2018

Thanks @geoffmcl for your help. I have managed to write a program using it.

I don't think I pointed out what you're saying I pointed out. Particularly I hadn't found any universal information about return values, only information specific to the parsing functions. The information I pointed to also said nothing about severe errors or negative numbers.

Perhaps, since that documentation does exist elsewhere, saying that it's useless is a little strong. But saying merely that it returns "An integer representing the status", and forcing you to dig for the documentation about the status convention, still seems less than excellent. It would be much more helpful to mention in that location what the values mean. Doing so would also increase consistency in the documentation, since the parsing documentation already says what the values (for the parsing functions) mean.

I can imagine that there would be an argument that the extra length or redundancy of information is undesirable, but if that's the case it doesn't make sense that the documentation for the parse functions has that information.

In the end I still maintain that there is an issue in the documentation that should be resolved.

I hope that helps you understand what my issue is. If not, feel free to ask more questions.

@TELunus
Copy link
Author

TELunus commented Mar 16, 2018

I should also point out that although this issue has the label "Technical support", what I'm saying is not "I can't figure out how to do this", but instead "The documentation provided does not clearly tell developers how to do this".

@geoffmcl
Copy link
Contributor

@TELunus I certainly agree the documentation can always be improved... and sorry I misread this a little... and glad you got a program working...

As you indicate, in the next section after Diagnostics and Repair, namely Document Parse, which technically should be read before getting to Diagnostics and Repair - How to re-order that? - each API has a verbose -

**Returns**
 Returns the highest of 2 indicating that errors were present in the document, 
1 indicating warnings, and 0 in the case of everything being okay.

But still does not mention that they can also return a negative errno in case of a severe error, like if passed a NULL instead of a TidyDoc, and other cases, like file does not exist, etc, etc...

And the next section Document Save Functions returns to the same unhelpful An integer representing the status....

I too guess the creators of the doxygen generated api docs, thus the comments, particularly in tidy.h, where most of this comes from, tried to strike a balance between lengthy, perhaps redundant information and sufficient information, and failed in this case...

Maybe at least the message could be extended to say An integer status - 0, 1, 2, or less than zero. See LibTidy -> Implement LibTidy section for meaning....

And I do not know enough about doxygen comments to be able to make that a cross document html link, but maybe that would be possible...

What do you think?

And would particularly like help from someone understanding more of doxygen comments concerning creating such a link, and maybe re-ordering the sections... Help needed... thanks...

@TELunus
Copy link
Author

TELunus commented Mar 22, 2018

@geoffmcl what you've got there looks to me like it would be good. Alternately perhaps something like this could be put in the diagnostics and repair section:

Returns
Returns the highest of 2 indicating that errors were present in the document,
1 indicating warnings, and 0 in the case of everything being okay, or a negative
number for a severe error (often taken from errno.h)

I would be happy with either of those.

@geoffmcl
Copy link
Contributor

@TELunus thank you for the feedback, much appreciated, but I do not think we have quite cracked this yet...

What do we have now in the documentation?

We have six(6) cases of -

 ** @result Returns the highest of `2` indicating that errors were present in
 **         the document, `1` indicating warnings, and `0` in the case of
 **         everything being okay.

Once in section Basic Operations, for tidyStatus, and 5 times in Document Parse. But this is incomplete in that it does not mention the possible less than zero error result...

Then we have ten(10) cases of -

 ** @result An integer representing the status.

This is in 2 sections: Diagnostics and Repair - 3 times, and Document Save Functions - 7 times.

Now, as you suggest, we could replace all sixteen cases with a full message -

 ** @result Returns the highest of 2 indicating that errors were present in the document, 
 **         1 indicating warnings, and 0 in the case of everything being okay, or a negative  
 **         number for a severe error (often taken from errno.h)

But somehow, to me that seems far too much repetition - looks ugly... but is fully correct...

And remember this is all C comments in the main tidy.h, adding to its size - a file included in every module... and shipped as part of the distribution...

Yes I know this is less important today with ever bigger hard-disks, and fast file I/O, file caching... but tidy.h is already over 100KB... 102,511 bytes in windows... and just FWIIW it is already 75% comments... do we need to add more is all I am saying...

That is why I chose a shorter sentence, plus a pointing to somewhere else...

And since these are separate section pages in the api docs, maybe the full paragraph you suggest be put at the top of 3 relevant page, like Diagnostics and Repair, Document Parse and Document Save Functions, with a wording like -

 ** Functions that return 'an integer status' will return the highest of 2 indicating that errors 
 ** were present in the document, 1 indicating warnings, and 0 in the case of everything being 
 ** okay, or a negative number for a severe error (often taken from errno.h)

Then each individual API function, well 15 of the 16 - can have say a simple, short -

 ** @result An integer status. See top.

Would exclude the one in Basic Operations ie. tidyStatus(doc) since this is the only 1 in that section, but with the message expanded to the full message...

Testing it all out -

  1. Changing all 16 places to the full correct message adds 2KB to the size of tidy.h. Just aout 2% - quite small really - but an addition none the less, to an already large file... 104,952 bytes...

  2. Changing one to be the full message, adding that full integer status message at the top of 3 sections, and changing 15 to the above See top. short message kept tidy.h at about the same 100KB... 102,592 bytes

So what do you and others think? Please give some feedback... I am really only 49:51 on this... thanks...

@TELunus
Copy link
Author

TELunus commented Mar 30, 2018

@geoffmcl I think like you I'm also not very sure which of those two options to take. It sounds like you lean a little toward option 2:

Changing one to be the full message, adding that full integer status message at the top of 3 sections, and changing 15 to the above See top. short message kept tidy.h at about the same 100KB... 102,592 bytes

I think I also lean a little toward that option. Should we wait for some more input, or should one of us get started making those changes?

@cmb69
Copy link
Contributor

cmb69 commented Oct 20, 2018

I see at least two possible solutions:

  • use the @snippetdoc command
  • do something like typedef tidy_status int, and use the new type as return type for the functions

@balthisar balthisar modified the milestones: 5.7, 5.9 Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants