Update aws_get_environment_value to return NULL for empty values

awslabs / aws-c-common

Core c99 package for AWS SDK for C. Includes cross-platform primitives, configuration, data structures, and error handling.

Apache License 2.0

262 stars 159 forks source link

Update aws_get_environment_value to return NULL for empty values #1141

Open waahm7 opened 3 months ago

waahm7 commented 3 months ago

Description of changes: aws_get_environment_value is error-prone because one of the frequent use cases is to check an environment variable, else check other source like config file. In that case, the previous implementation required us to check three cases:

NULL -> check the alternative
Empty -> destroy and check the alternative
Available -> use that Since we don't need to differentiate between empty values and unset values, treat empty environment values as unset.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.84%. Comparing base (2add521) to head (e5e7776).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1141 +/- ## ========================================== + Coverage 83.82% 83.84% +0.01% ========================================== Files 57 57 Lines 5991 5998 +7 ========================================== + Hits 5022 5029 +7 Misses 969 969 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bretambrose commented 3 months ago

  Since we don't need to differentiate between empty values and unset values, treat empty environment values as unset.

This is a pretty big assumption since you're not only asserting it for right now, but for everything that comes in the future too. I don't really agree with this.