TokTok / c-toxcore

The future of online communications.
https://tox.chat
GNU General Public License v3.0
2.25k stars 284 forks source link

chore: Small API doc fixes #2735

Closed nurupo closed 6 months ago

nurupo commented 6 months ago

API docs are a bit inconsistent:

  1. sometimes use "tox" instead of "Tox"
  2. sometimes use "null" instead of "NULL"
  3. sometimes calls strings NUL-terminated, other times NULL terminated
  4. sometimes comments go over 80 columns
  5. sometimes @param are listed after @return
  6. sometimes it's a function, other times it's a `function`, and yet other times it's a `function()`, same for parameters or `parameters` -- oh, and sometimes a single comment mixes multiple styles! (e.g. tox_group_get_topic)
  7. NGC functions doesn't use @breif for some reason when all other functions do. Why?
  8. some functions don't document their parameters, e.g. tox_group_set_topic, while others document even the obvious most parameters.

This PR addresses only the first 4 points, and one case of the 5th point that I have noticed by an accident.

5-8 points are not addressed. Would be nice if someone could address those.


This change is Reviewable

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 73.04%. Comparing base (ad4921d) to head (a3d1b85).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2735 +/- ## ========================================== - Coverage 73.12% 73.04% -0.08% ========================================== Files 149 149 Lines 30516 30516 ========================================== - Hits 22314 22290 -24 - Misses 8202 8226 +24 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nurupo commented 6 months ago

@JFreegman What does it mean when NGC says "core error"? I'm confused by it being named "core". Does that mean it's a toxcore error? Or perhaps it means "a fatal error" -- something horribly went wrong, without specifying what it is, like some sort of a general error? Maybe it's better not to call it a core error but to come up with some other synonym / wording?

https://github.com/TokTok/c-toxcore/blob/3e05824b80eb9bee33e8254cba0780d84c522182/toxcore/tox.h#L3641-L3644

https://github.com/TokTok/c-toxcore/blob/3e05824b80eb9bee33e8254cba0780d84c522182/toxcore/tox.h#L3738-L3741

https://github.com/TokTok/c-toxcore/blob/3e05824b80eb9bee33e8254cba0780d84c522182/toxcore/tox.h#L4890-L4893

nurupo commented 6 months ago

@iphydf const Tox_System *operating_system is located under the experimental comment disclaimer, sandwiched between two experimental_* options, yet it is not marked as experimental, i.e. it's not named experimental_operating_system. Does that mean that it's not experimental and a part of the supported API, as per the experimental comment disclaimer? Or did we just forget to prefix its name with experimental_?

https://github.com/TokTok/c-toxcore/blob/3e05824b80eb9bee33e8254cba0780d84c522182/toxcore/tox.h#L662-L692

iphydf commented 6 months ago

operating_system is experimental.

nurupo commented 6 months ago

@iphydf well, it's not obvious.

 * These options are experimental, so avoid writing code that depends on 
 * them. Options marked "experimental" may change their behaviour or go away 
 * entirely in the future, or may be renamed to something non-experimental 
 * if they become part of the supported API. 

I don't see it being marked as experimental in any way.

Based on that comment, I would think that it got "renamed to something non-experimental" and "become part of the supported API".

JFreegman commented 6 months ago

What does it mean when NGC says "core error"? I'm confused by it being named "core". Does that mean it's a toxcore error? Or perhaps it means "a fatal error" -- something horribly went wrong, without specifying what it is, like some sort of a general error? Maybe it's better not to call it a core error but to come up with some other synonym / wording?

In the case of join and rejoin, it means Messenger.c failed to create a friend connection for some reason (groupchats use a friend connection for onion/DHT stuff). In the case of invite accept, see: https://github.com/TokTok/c-toxcore/pull/2736

nurupo commented 6 months ago

@JFreegman that's one, what about the other two core errors? No plan to rename them to "initiation", "general", "fatal" or anything else, keeping them as "core" errors?

JFreegman commented 6 months ago

@JFreegman that's one, what about the other two core errors? No plan to rename them to "initiation", "general", "fatal" or anything else, keeping them as "core" errors?

I'm sort of indifferent. I think "core" conveys about as much information as anything else. You can change it if you want.

nurupo commented 6 months ago

Alright, if no one else has an issue with it then I guess let's keep them as "core".

JFreegman commented 6 months ago

Is there a reason we're putting this in the .19 release? Documentation cleanup isn't critical for the release is it?

nurupo commented 6 months ago

@JFreegman The very first paragraphs of tox.h talk about apidsl, which we no longer use, about tox.in.h, which doesn't exist, about old style callbacks, which also don't exist anymore. It makes sense to correct these points before a release. That's what the very first commit addresses, and what the PR was going to be about initially. All the following commits are mostly cleanups of things that I noticed after making the first commit rather than being corrections. Aside perhaps from iphy forgetting to s/events_alloc/internal/ in tox.h, which is another correction this PR makes that was discovered later. So anyway, it's not just cleanups and I think it makes sense for .19.

The PR is ready to be merged. It looks like we are waiting on @iphydf to mark his comments as resolved and that's it. Any day now. Weird how he approved the PR but didn't resolve them. Or perhaps he expects me to mark them as resolved?

JFreegman commented 6 months ago

Or perhaps he expects me to mark them as resolved?

I usually mark them as resolved myself when I resolve them.

iphydf commented 6 months ago

Yes please resolve it for me. I'm on the road and don't have a laptop so I'm doing this all on the phone, where it's hard to find that comment. I approved it, it looks good to me, no comments need addressing.