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

Fix appsec lint #3021

Merged
merged 1 commit into from
Jan 8, 2025
Merged

Fix appsec lint #3021

merged 1 commit into from
Jan 8, 2025

Conversation

estringana
Copy link
Contributor

Description

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 72.93%. Comparing base (906cbc5) to head (19874c1).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
appsec/src/extension/network.c 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3021      +/-   ##
============================================
- Coverage     74.80%   72.93%   -1.87%     
  Complexity     2781     2781              
============================================
  Files           112      139      +27     
  Lines         11017    15166    +4149     
  Branches          0     1022    +1022     
============================================
+ Hits           8241    11062    +2821     
- Misses         2776     3552     +776     
- Partials          0      552     +552     
Flag Coverage Δ
appsec-extension 67.99% <0.00%> (?)
tracer-php 74.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
appsec/src/extension/commands_helpers.c 63.02% <ø> (ø)
appsec/src/extension/network.c 44.69% <0.00%> (ø)

... and 25 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 906cbc5...19874c1. Read the comment docs.

@estringana estringana force-pushed the estringana/fix-appsec-lint branch 2 times, most recently from bc883bc to 56a7317 Compare January 7, 2025 15:06
Copy link
Contributor

@Anilm3 Anilm3 left a comment

Choose a reason for hiding this comment

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

Generally I'd suggest specifying the linter check when using NOLINT or NOLINTNEXTLINE.

@@ -218,7 +218,7 @@ dd_result dd_conn_sendv(dd_conn *nonnull conn, zend_llist *nonnull iovecs)
mlog_err(dd_log_info, "Error writing %zu bytes to helper", total);
efree(iovs);
return dd_network;
} else if (written == 0) {
} else if (written == 0) { // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

you can take out the else so that // NOLINT is not needed

@estringana estringana marked this pull request as ready for review January 8, 2025 07:19
@estringana estringana requested a review from a team as a code owner January 8, 2025 07:20
@estringana estringana force-pushed the estringana/fix-appsec-lint branch from 56a7317 to 19874c1 Compare January 8, 2025 08:20
@estringana estringana merged commit 55377fe into master Jan 8, 2025
677 of 725 checks passed
@estringana estringana deleted the estringana/fix-appsec-lint branch January 8, 2025 09:05
@github-actions github-actions bot added this to the 1.6.0 milestone Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants