Skip to content

chore: Reformat sources with astyle.#2629

Merged
1 commit merged into
TokTok:masterfrom
iphydf:reformat
Feb 2, 2024
Merged

chore: Reformat sources with astyle.#2629
1 commit merged into
TokTok:masterfrom
iphydf:reformat

Conversation

@iphydf
Copy link
Copy Markdown
Member

@iphydf iphydf commented Feb 2, 2024

Restyled astyle is fixed now.


This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Feb 2, 2024
@iphydf iphydf marked this pull request as ready for review February 2, 2024 00:11
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 2, 2024

Codecov Report

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

Comparison is base (4359e3a) 73.71% compared to head (4e2dba4) 73.82%.

Files Patch % Lines
toxcore/TCP_connection.c 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2629      +/-   ##
==========================================
+ Coverage   73.71%   73.82%   +0.10%     
==========================================
  Files         148      148              
  Lines       30424    30419       -5     
==========================================
+ Hits        22428    22456      +28     
+ Misses       7996     7963      -33     

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

@iphydf iphydf force-pushed the reformat branch 2 times, most recently from 23a92cb to c1e13e3 Compare February 2, 2024 00:44
@iphydf iphydf force-pushed the reformat branch 7 times, most recently from a3a37dc to 62f6790 Compare February 2, 2024 01:30
Restyled astyle is fixed now.
@nurupo
Copy link
Copy Markdown
Member

nurupo commented Feb 2, 2024

(Annoying how GitHub's "Hide whitespace" doesn't hide files with the only change being removal of empty lines.)

Comment thread other/astyle/astylerc
Comment thread toxcore/events/file_recv.c
@pull-request-attention pull-request-attention Bot assigned iphydf and unassigned nurupo Feb 2, 2024
Comment thread other/astyle/astylerc
--add-braces
--convert-tabs
--max-code-length=120
--max-code-length=200
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the reason for the line length increase? (Please don't say that it's a new 4k monitor you got :D)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mainly that astyle is doing a really poor job at breaking argument lists, so I'd rather make this manual. I'd make it 1000 if I could, but astyle limits this to 200.

Example:

-            ck_assert_msg(get_close_nodes(subtoxes[i]->dht, dht_get_self_public_key(subtoxes[i]->dht), nodes, net_family_unspec(), true,
+            ck_assert_msg(get_close_nodes(subtoxes[i]->dht, dht_get_self_public_key(subtoxes[i]->dht), nodes, net_family_unspec(),
+                                          true,
                                           true) > 0,

-bool tox_pass_encrypt(const uint8_t plaintext[], size_t plaintext_len, const uint8_t passphrase[], size_t passphrase_len,
+bool tox_pass_encrypt(const uint8_t plaintext[], size_t plaintext_len, const uint8_t passphrase[],
+                      size_t passphrase_len,
                       uint8_t ciphertext[/*! plaintext_len + TOX_PASS_ENCRYPTION_EXTRA_LENGTH */], Tox_Err_Encryption *error);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But that now open the gate for 200-wide if-statements, comments, etc., and 200 columns is a bit too wide, that's a big chunk of a 1080p screen at 9pt monospace (the whole screen is around 270 columns, give or take).

@toktok-releaser toktok-releaser closed this pull request by merging all changes into TokTok:master in b7404f2 Feb 2, 2024
@nurupo
Copy link
Copy Markdown
Member

nurupo commented Feb 2, 2024

Uh...

@nurupo
Copy link
Copy Markdown
Member

nurupo commented Feb 2, 2024

@toktok-releaser We are still not done discussing here.

@iphydf
Copy link
Copy Markdown
Member Author

iphydf commented Feb 2, 2024

@toktok-releaser We are still not done discussing here.

#2621 was approved and merged. @toktok-releaser can't really know that your approval is actually not approving the full pr and needs to wait for a base pr review first.

@pull-request-attention pull-request-attention Bot assigned nurupo and unassigned iphydf Feb 2, 2024
@iphydf iphydf deleted the reformat branch February 2, 2024 11:38
@nurupo
Copy link
Copy Markdown
Member

nurupo commented Feb 2, 2024

@toktok-releaser can't really know that your approval is actually not approving the full pr and needs to wait for a base pr review first.

@iphydf the same can be said for @nurupo. Although it wasn't the case here since I knew of both PRs, in a hypothetical scenario, I might not know that PR2 is based on an earlier PR1 (as I might have not seen PR1 yet, maybe I'm just going over my GitHub emails and PR2 was the first I opened). PR2 looks good to me and I approve PR2, which results in PR2 and PR1 getting merged, without me knowing that there is a separate PR1 that is being discussed by someone else. How do we prevent something like this in the future? Even more generally, how do we prevent a PR2, which based on a PR1, from being merged before the PR1 does?

Should we disallow basing PRs on other PRs?

Can the PR2 author block their own PR? That way others could review it and approve it, with the author coordinating for the PR2 to be merged after the base PR1 does? Or perhaps better yet, just keep the PR2 marked as WIP until the base PR1 is merged?


Btw, what happened in this case is that I forgot that @toktok-releaser exists and may auto-merge a PR, as it sometimes auto-merges PRs but other times not (can't merge in --ff-only mode so it bails?), so I thought it would be manually merged by @iphydf who would do the right thing and wait on this discussion to finish first.


I want to point out that:

  • After you (the generic "you", e.g. me, you, someone else) request changes on a PR and are happy with how that got addressed, it's natural to approve the PR -- Reviewable and GitHub nudge you into doing it after you resolve all of the discussions. Postponing approval of the PR until some other PR is merged is somewhat of an unnatural workflow.

  • In addition to that, postponing the PR approval might lead to you forgetting that you were happy with this PR, depending on how long it's postponed, i.e. how long the base PR discussion goes for. (Though, I guess, one could leave a comment for the future selves saying that they were happy with the PR, without clicking on Approve.)

  • Not postponing the approval can also be problematic -- depending on the outcome of the base PR discussion, the base PR might change drastically, which might also drastically change the current PR as it's based on the base, so it would need to be re-reviewed again but because it's already been approved it can get merged (even auto-merged by @toktok-releaser) without any extra approvals despite the diff being drastically different now.

All of this makes me steer more towards the idea of marking PRs based on other PRs as WIP.

@pull-request-attention pull-request-attention Bot assigned iphydf and unassigned nurupo Feb 2, 2024
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