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: Ensure handler params are named after callback params. #2581

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 17, 2024

This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Jan 17, 2024
@iphydf iphydf marked this pull request as ready for review January 17, 2024 22:36
Copy link

codecov bot commented Jan 17, 2024

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (c650d9d) 66.84% compared to head (89957be) 66.86%.

Files Patch % Lines
toxav/toxav.c 57.14% 3 Missing ⚠️
toxcore/Messenger.c 94.73% 3 Missing ⚠️
toxcore/bin_unpack.c 0.00% 3 Missing ⚠️
toxav/groupav.c 94.28% 2 Missing ⚠️
toxav/msi.c 33.33% 2 Missing ⚠️
toxcore/bin_pack.c 80.00% 1 Missing ⚠️
toxcore/friend_connection.c 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2581      +/-   ##
==========================================
+ Coverage   66.84%   66.86%   +0.02%     
==========================================
  Files         148      148              
  Lines       30332    30330       -2     
==========================================
+ Hits        20274    20279       +5     
+ Misses      10058    10051       -7     

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

@iphydf iphydf merged commit 89957be into TokTok:master Jan 18, 2024
55 of 56 checks passed
@iphydf iphydf deleted the callback-params branch January 18, 2024 11:15
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I had a chuckle at a variable named todo. Then I started reading further and fell into despair from the inconsistency of things.

After this PR is merged, some files, like groupav.c, will introduce "conference" that replaces "group", but not everywhere in the file since the PR does only very specific, low scope, edit. So you end up with "conference" being used in some places and "group" in others. Similar with "friend number" and "peer number". The PR didn't even edit the function docs for the functions it changed so that they match the updated function argument names -- they still talk about groups and friend numbers and not conferences and peer numbers. This leaves groupav.c in such an inconsistent naming convention state that I'm denying and not even reviewing the rest of the PR.

Reviewed 11 of 33 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @iphydf)


toxav/groupav.h line 24 at r1 (raw file):

/** @brief Create and connect to a new toxav group.
 *
 * @return group number on success.

The documentation still says "group number" and refers to conferences as groups. This and other functions' documentation.


toxav/groupav.h line 34 at r1 (raw file):

 * @retval -1 on failure.
 */
int join_av_groupchat(const Logger *log, Tox *tox, Group_Chats *g_c, uint32_t friendnumber, const uint8_t *data,

No friendnumber -> peernumber here?


toxav/groupav.h line 34 at r1 (raw file):

 * @retval -1 on failure.
 */
int join_av_groupchat(const Logger *log, Tox *tox, Group_Chats *g_c, uint32_t friendnumber, const uint8_t *data,

No renaming this into join_av_conference()?

@nurupo
Copy link
Member

nurupo commented Jan 18, 2024

Welp, this got merged just as I was reviewing it.

@Green-Sky
Copy link
Member

ah sorry about that. in general, the av code is not maintained as well as core. we are basically waiting with consistent larger changes for #1431 to be merged, but that wont happen until after 0.2.19.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants