search

influxdata / influxdb-client-php

InfluxDB (v2+) Client Library for PHP
https://influxdata.github.io/influxdb-client-php/
MIT License
151 stars 47 forks source link

Throw exception if no field is provided when calling toLineProtocol() #150

Open baptgb opened 1 year ago

baptgb commented 1 year ago

Closes #149

Proposed Changes

toLineProtocol() will throw an exception if no field is provided. This prevents the data from being silently converted to null and discarded, thereby providing the developer with a clear error message, as fields are mandatory.

Checklist

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (22f5d55) 74.86% compared to head (abf6b1a) 74.86%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #150 +/- ## ========================================= Coverage 74.86% 74.86% Complexity 424 424 ========================================= Files 25 25 Lines 1094 1094 ========================================= Hits 819 819 Misses 275 275 ``` | [Files Changed](https://app.codecov.io/gh/influxdata/influxdb-client-php/pull/150?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=influxdata) | Coverage Δ | | |---|---|---| | [src/InfluxDB2/Point.php](https://app.codecov.io/gh/influxdata/influxdb-client-php/pull/150?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=influxdata#diff-c3JjL0luZmx1eERCMi9Qb2ludC5waHA=) | `100.00% <100.00%> (ø)` | |

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

baptgb commented 1 year ago

Hi,

Thanks for the issue and taking the time to put up a PR. We would like to maintain the default behavior of not throwing an exception. The change could cause issues for users who upgrade and suddenly start getting exceptions.

Is there a way you could create an option in the client to turn on this new behavior?

Hi, Thank you for your review. I understand that introducing this change could lead to breaking issues for existing implementations. However, in my opinion, keeping this option deactivated by default somewhat diminishes its utility. I would suggest enabling it by default in the next release where breaking changes are announced.

I propose adding the silentFailOnEmptyField option to the Client, defaulting it to true. I'll go ahead and make the change if you agree.

powersj commented 1 year ago

I would suggest enabling it by default in the next release where breaking changes are announced.

Unfortunately, at this late in the life-cycle we do not want introduce this type of change.