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.

Sign up for GitHub

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

Jump to bottom

feat: #145 try to convert non-string-tags to strings #146

Merged
merged 3 commits into from
Jan 10, 2024
Merged

feat: #145 try to convert non-string-tags to strings #146

merged 3 commits into from
Jan 10, 2024

Conversation

mkrauser
Copy link
Contributor

@mkrauser mkrauser commented Jul 28, 2023 •
edited
Loading

Closes #145

Proposed Changes

  • add a TypeHint to Point->addTag(). PHP will auto-convert scalars and stringable values. Null-Values are handled like before
  • auto-convert scalars, objects with __toString() and stringable objects
  • if the value cannot be converted, throw an exception

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • make test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@mkrauser mkrauser force-pushed the issue-145-convert-tags-to-strings branch 3 times, most recently from 2252cff to 872fd1f Compare July 28, 2023 22:49
bednar
bednar requested changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@mkrauser thanks for your PR 👍

There are a few requirements that must be satisfied before we accept the PR:

src/InfluxDB2/Point.php Outdated
Comment on lines 173 to 175
throw new \InvalidArgumentException(
sprintf('Tag value for key %s cannot be converted to string', $key)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, change the exception to a warning. This is a breaking change that could cause problems for other users.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mkrauser mkrauser Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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 mkrauser force-pushed the issue-145-convert-tags-to-strings branch 7 times, most recently from 4d93aef to 9930820 Compare January 5, 2024 11:58
@mkrauser
feat: try to convert non-string tags to strings
08dbc14 
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 mkrauser force-pushed the issue-145-convert-tags-to-strings branch from 9930820 to 08dbc14 Compare January 5, 2024 12:05
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (22f5d55) 74.86% compared to head (08dbc14) 75.00%.

❗ Current head 08dbc14 differs from pull request most recent head 2eb05eb. Consider uploading reports for the commit 2eb05eb to get more accurate results

❗ 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.
📢 Have feedback on the report? Share it here.

bednar
bednar requested changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! 👍 Before we can proceed with accepting it, there are a few requirements that need to be met:

src/InfluxDB2/Point.php Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
mkrauser and others added 2 commits January 8, 2024 21:39
@mkrauser @bednar
Update CHANGELOG.md
2eb05eb 
Co-authored-by: Jakub Bednář <[email protected]>
@mkrauser @bednar
Adjusted phpdoc
665f8b1 
Co-authored-by: Jakub Bednář <[email protected]>
bednar
bednar approved these changes Jan 10, 2024
View reviewed changes
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

@mkrauser thanks again for your PR 👍

LGTM 🚀

@bednar bednar changed the title #145 try to convert non-string-tags to strings feat: #145 try to convert non-string-tags to strings Jan 10, 2024
@bednar bednar merged commit bac7914 into influxdata:master Jan 10, 2024
11 of 12 checks passed
@bednar bednar added this to the 3.5.0 milestone Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pprkut pprkut pprkut left review comments

@bednar bednar bednar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.5.0
Development

Successfully merging this pull request may close these issues.

Try to convert non-string-tags to strings or throw an Exception
4 participants
@mkrauser @codecov-commenter @pprkut @bednar