feat: #145 try to convert non-string-tags to strings #146
Conversation
mkrauser force-pushed the issue-145-convert-tags-to-strings branch 3 times, most recently from 2252cff to 872fd1f Compare 
src/InfluxDB2/Point.php Outdated
| throw new \InvalidArgumentException( | ||
| sprintf('Tag value for key %s cannot be converted to string', $key) | ||
| ); |
There was a problem hiding this comment.
Please, change the exception to a warning. This is a breaking change that could cause problems for other users.
There was a problem hiding this comment.
I would also suggest adding a unit test that verifies the exception is actually thrown. Because looking at the code, the parameter type hint should prevent the entire if-statement from ever executing.
There was a problem hiding this comment.
Please, change the exception to a warning. This is a breaking change that could cause problems for other users.
@bednar done
I would also suggest adding a unit test that verifies the exception is actually thrown. Because looking at the code, the parameter type hint should prevent the entire if-statement from ever executing.
If only addTag is used to add tags, then you are right. But it is also possible to pass them in the constructor. I've added unit tests for this.
Another suggestion would be to add array typehints in the constructor, but I want to keep it small for now.
mkrauser force-pushed the issue-145-convert-tags-to-strings branch 7 times, most recently from 4d93aef to 9930820 Compare 
BREAKING CHANGE: addTags only accepts null and strings as $value. And warning is triggered when a non-string value cannot be converted to string Refs: influxdata#145
mkrauser force-pushed the issue-145-convert-tags-to-strings branch from 9930820 to 08dbc14 Compare 
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@ ## master #146 +/- ## ============================================ + Coverage 74.86% 75.00% +0.13% - Complexity 424 432 +8 ============================================ Files 25 25 Lines 1094 1100 +6 ============================================ + Hits 819 825 +6 Misses 275 275 ☔ View full report in Codecov by Sentry. |
bednar left a comment
bednar left a comment There was a problem hiding this comment.
Thank you for your PR! 👍 Before we can proceed with accepting it, there are a few requirements that need to be met:
Co-authored-by: Jakub Bednář <jakub.bednar@gmail.com>
Co-authored-by: Jakub Bednář <jakub.bednar@gmail.com>
bednar changed the title 4 participants
Closes #145
Proposed Changes
Checklist
make testcompletes successfully