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

cleanup: align group send err enum order #2731

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Mar 8, 2024

This change is Reviewable

@Green-Sky Green-Sky added this to the v0.2.19 milestone Mar 8, 2024
@Green-Sky Green-Sky added the api break Change breaks API or ABI label Mar 8, 2024
@Green-Sky Green-Sky force-pushed the fix_group_err_order branch from b1705a6 to 0f8fb2d Compare March 8, 2024 09:42
@Green-Sky Green-Sky force-pushed the fix_group_err_order branch from 0f8fb2d to 2457125 Compare March 8, 2024 09:47
@Green-Sky Green-Sky marked this pull request as ready for review March 8, 2024 09:52
Copy link

codecov bot commented Mar 8, 2024

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

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

@Green-Sky Green-Sky merged commit 2457125 into TokTok:master Mar 8, 2024
62 of 63 checks passed
@zoff99
Copy link

zoff99 commented Mar 9, 2024

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

zoff99 commented Mar 9, 2024

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

iphydf commented Mar 9, 2024

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

zoff99 commented Mar 9, 2024

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

iphydf commented Mar 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api break Change breaks API or ABI
Development

Successfully merging this pull request may close these issues.

3 participants