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

Add clang formatting script and format all sample and src files #1095

Open
wants to merge 13 commits into
base: develop-pre-3.4.1
Choose a base branch
from

Conversation

stefankiesz
Copy link
Contributor

@stefankiesz stefankiesz commented Oct 28, 2023

Added the CLANG formatting guidelines used in Producer C SDK, formatted all files in the sample and src directories.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@stefankiesz stefankiesz marked this pull request as draft October 30, 2023 18:35
@stefankiesz stefankiesz marked this pull request as ready for review November 10, 2023 23:52
@stefankiesz stefankiesz changed the title Add clang formatting script and format all smaple and src files Add clang formatting script and format all sample, tst, and src files Nov 10, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8002fdf) 16.34% compared to head (6915096) 16.39%.

❗ Current head 6915096 differs from pull request most recent head 37577d4. Consider uploading reports for the commit 37577d4 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1095      +/-   ##
===========================================
+ Coverage    16.34%   16.39%   +0.05%     
===========================================
  Files           51       51              
  Lines         6846     6849       +3     
===========================================
+ Hits          1119     1123       +4     
+ Misses        5727     5726       -1     

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

Comment on lines +87 to +91
gchar* stream_name;
gchar* user_agent;
guint retention_period_hours;
gchar* kms_key_id;
STREAMING_TYPE streaming_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to keep it how it was before, I think the before version here is clearer. We should stick to one way and be consistent, the consistency is most important.

Copy link
Contributor Author

@stefankiesz stefankiesz Nov 30, 2023

Choose a reason for hiding this comment

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

In Producer C, we do use this same style of not having spaces added to align variable names - there are spaces for "#define"s though. I tested and searched around to find that the way clang-format works is it parses all the code and then puts it back together according to the configured format, so there is no way to remove this formatting. There may be a formatting rule that would add this spacing between variable type and name, but it would then apply to all variables. The two solutions I see are to either use another formatter, or place the following around code we do not want to format:

// clang-format off
...
// clang-format on

@stefankiesz stefankiesz changed the title Add clang formatting script and format all sample, tst, and src files Add clang formatting script and format all sample and src files Dec 5, 2023
Copy link
Contributor

@niyatim23 niyatim23 left a comment

Choose a reason for hiding this comment

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

If we introduce the files for formatting, and add a build in the CI but don't update our codebase to reflect the format, the check in the CI will fail. Is there any specific reason you removed the formatting from all the files you had added previously?

@stefankiesz
Copy link
Contributor Author

@niyatim23 I was testing whether it would affect the failing address sanitizer CI job. Just reverted those commits.

@sirknightj sirknightj added the CI label Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants