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

chore: Small API doc fixes #2735

Merged
merged 8 commits into from
Mar 14, 2024
Merged

chore: Small API doc fixes #2735

merged 8 commits into from
Mar 14, 2024

Conversation

nurupo
Copy link
Member

@nurupo nurupo commented Mar 10, 2024

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

@nurupo nurupo added this to the v0.2.19 milestone Mar 10, 2024
Copy link

codecov bot commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

toxcore/tox.h Outdated Show resolved Hide resolved
toxcore/tox.h Outdated Show resolved Hide resolved
toxcore/tox.h Outdated Show resolved Hide resolved
toxcore/tox.h Outdated Show resolved Hide resolved
toxcore/tox.h Outdated Show resolved Hide resolved
@nurupo
Copy link
Member Author

nurupo commented Mar 10, 2024

@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?

c-toxcore/toxcore/tox.h

Lines 3641 to 3644 in 3e05824

/**
* There was a core error when initiating the group.
*/
TOX_ERR_GROUP_JOIN_CORE,

c-toxcore/toxcore/tox.h

Lines 3738 to 3741 in 3e05824

/**
* There was a core error when initiating the group.
*/
TOX_ERR_GROUP_RECONNECT_CORE,

c-toxcore/toxcore/tox.h

Lines 4890 to 4893 in 3e05824

/**
* There was a core error when initiating the group.
*/
TOX_ERR_GROUP_INVITE_ACCEPT_CORE,

@pull-request-attention pull-request-attention bot assigned iphydf and unassigned nurupo Mar 10, 2024
@nurupo
Copy link
Member Author

nurupo commented Mar 10, 2024

@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_?

c-toxcore/toxcore/tox.h

Lines 662 to 692 in 3e05824

/**
* 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.
*/
/**
* Make public API functions thread-safe using a per-instance lock.
*
* Default: false.
*/
bool experimental_thread_safety;
/**
* Low level operating system functionality such as send/recv, random
* number generation, and memory allocation.
*/
const Tox_System *operating_system;
/**
* Enable saving DHT-based group chats to Tox save data (via `tox_get_savedata`).
* This format will change in the future, so don't rely on it.
*
* As an alternative, clients can save the group chat ID in client-owned
* savedata. Then, when the client starts, it can use `tox_group_join`
* with the saved chat ID to recreate the group chat.
*
* Default: false.
*/
bool experimental_groups_persistence;
};

@iphydf
Copy link
Member

iphydf commented Mar 11, 2024

operating_system is experimental.

@nurupo
Copy link
Member Author

nurupo commented Mar 11, 2024

@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".

@nurupo nurupo marked this pull request as ready for review March 11, 2024 00:23
@JFreegman
Copy link
Member

JFreegman commented Mar 11, 2024

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: #2736

@nurupo
Copy link
Member Author

nurupo commented Mar 12, 2024

@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?

nurupo added 3 commits March 11, 2024 23:21
There is no APIDSL so it shouldn't be mentioned, nor are there old style
callbacks that take user data when registering them.
@JFreegman
Copy link
Member

@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.

@pull-request-attention pull-request-attention bot assigned nurupo and unassigned iphydf Mar 12, 2024
@nurupo
Copy link
Member Author

nurupo commented Mar 12, 2024

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

toxcore/tox.h Outdated Show resolved Hide resolved
@nurupo nurupo force-pushed the fix-api-docs branch 2 times, most recently from 6ba2320 to 682cc87 Compare March 13, 2024 03:10
nurupo added 2 commits March 12, 2024 23:19
Toxcore public header documentation suffers from an identity crisis. It calls
itself three different things: Tox, toxcore and Core, depending on the
time period the comment was written in, the author of the comment,
whether the comment refers to the Tox instance or the Tox[core] library
(it doesn't seem like anything refers to the Tox protocol?), and maybe
some other nuances.

No one really calls it Core though, so this commit replaces the use of
Core with Tox/toxcore, and edits other instances of the documentation
referring to the library where I thought rewording was in order, though
in the end it's more of a subjective personal preference.
@JFreegman
Copy link
Member

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

@nurupo
Copy link
Member Author

nurupo commented Mar 14, 2024

@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
Copy link
Member

Or perhaps he expects me to mark them as resolved?

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

@iphydf
Copy link
Member

iphydf commented Mar 14, 2024

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.

@nurupo nurupo merged commit a3d1b85 into TokTok:master Mar 14, 2024
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants