search

TokTok / c-toxcore

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

cleanup: align group send err enum order #2731

Closed Green-Sky closed 3 months ago

Green-Sky commented 3 months ago

This change is Reviewable

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 72.99%. Comparing base (b03b571) to head (2457125).

Files Patch % Lines
toxcore/tox_api.c 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2731 +/- ## ========================================== - Coverage 73.14% 72.99% -0.15% ========================================== Files 149 149 Lines 30516 30516 ========================================== - Hits 22320 22276 -44 - Misses 8196 8240 +44 ```

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

zoff99 commented 3 months ago

we should pin enums with values. now this PR changes the actual int value of the enum, doesnt it? it changes from 8 to 5. and a bunch of others also change with it

zoff99 commented 3 months ago

@iphydf @Green-Sky can't we do something like this (at least of all public enums in tox.h and toxav.h) :

typedef enum Tox_Err_Group_Send_Private_Message {

    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_OK = 0,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_GROUP_NOT_FOUND = 1,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_PEER_NOT_FOUND = 2,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_TOO_LONG = 3,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_EMPTY = 4,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_BAD_TYPE = 5,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_PERMISSIONS = 6,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_FAIL_SEND = 7,
    TOX_ERR_GROUP_SEND_PRIVATE_MESSAGE_DISCONNECTED = 8

} Tox_Err_Group_Send_Private_Message;

then we can reorder how we like. with OK always must be (zero) "0" is there a downside to that?

iphydf commented 3 months ago

Enums can't be removed or reordered without breaking API or ABI, correct. This is why we're doing it now, before the release. What would be improved by pinning the numbers?

zoff99 commented 3 months ago

other question is, what would be worse? with pinning the numbers will be fixed, even when we would delete an entry in the enum. now you can't delete an entry without having to break things (unless its the last entry)

iphydf commented 3 months ago

With numbers, you can't delete members either, because each name is part of the API.