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: More fool-proof error message handling #155

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

RobQuistNL
Copy link
Contributor

@RobQuistNL RobQuistNL commented Jun 12, 2024

Proposed Changes

Changes the error thrown from something like this;

ErrorException
Undefined property: stdClass::$message

to something like this;

InfluxDB2\ApiException 
[500] Error connecting to the API (http://influxdb:8086/api/v2/query?org=-)(type error 1:35-1:76: function does not take a parameter "end", required params (start))

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)

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.09%. Comparing base (443c62c) to head (c829a1b).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #155      +/-   ##
============================================
+ Coverage     75.00%   75.09%   +0.09%     
- Complexity      432      433       +1     
============================================
  Files            25       25              
  Lines          1100     1104       +4     
============================================
+ Hits            825      829       +4     
  Misses          275      275              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Your message effectively requests additional work but can be polished for clarity and tone. Here's a refined version:


@RobQuistNL, thank you for your PR! 👍

Could you please add tests for your changes? This will aid in future maintenance and ensure that no regressions occur.

@bednar bednar changed the title More fool-proof error message handling fix: More fool-proof error message handling Jun 13, 2024
@RobQuistNL RobQuistNL requested a review from bednar June 13, 2024 09:29
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.

Thanks again for your PR 👍. Everything looks good, can you just satisfy our checklist (CLA and update CHANGELOG.md)?

image

@RobQuistNL
Copy link
Contributor Author

Thanks @bednar :)

I think I did all those things. Maybe best to add these requirements into the Contributing section or in a CONTRIBUTING.md file so in the future people spot it easier up front.

Thanks for reviewing so promptly!

@RobQuistNL RobQuistNL requested a review from bednar June 13, 2024 09:51
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.

LGTM 🚀

@bednar bednar added this to the 3.6.0 milestone Jun 13, 2024
@bednar bednar merged commit 0417d6e into influxdata:master Jun 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants