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: return str instead of urllib3.HTTPResponse for `InfluxDBClie… #606

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jules-ch
Copy link
Contributor

Closes #569

Proposed Changes

As it is done in the Async version of the client, return str response data as per the documentation.

Checklist

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

@jules-ch jules-ch force-pushed the 569-queryraw-return-type branch 2 times, most recently from 6ae3a67 to 0a4ae41 Compare August 22, 2023 19:01
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2f9e5ed) 90.30% compared to head (0a4ae41) 90.31%.
Report is 8 commits behind head on master.

❗ Current head 0a4ae41 differs from pull request most recent head a05c5b0. Consider uploading reports for the commit a05c5b0 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     #606   +/-   ##
=======================================
  Coverage   90.30%   90.31%           
=======================================
  Files          39       39           
  Lines        3456     3458    +2     
=======================================
+ Hits         3121     3123    +2     
  Misses        335      335           

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

@jules-ch
Copy link
Contributor Author

I had to use .data, using .read function with debug resulted with a bug when the response already read by the debugger it returned an empty string.
I'd have to check it is not the case with the async function.

@jules-ch
Copy link
Contributor Author

Ready for review

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.

Hi @jules-ch,

Thank you for your PR. Could you please update the CHANGELOG.md to indicate that this is a breaking change and provide guidance on how users should address it?

Thanks!

@jules-ch
Copy link
Contributor Author

Do you need me to introduce this as a BREAKING CHANGE.
Just want to wonfirm this, since the function was not returning the documented type in the first place.

I'll document it as a Breaking change for now

@jules-ch jules-ch force-pushed the 569-queryraw-return-type branch 2 times, most recently from f024155 to cb5ff71 Compare November 11, 2023 15:51
@jules-ch jules-ch force-pushed the 569-queryraw-return-type branch from cb5ff71 to f0c7ef4 Compare November 11, 2023 15:52
@jules-ch jules-ch requested a review from bednar November 11, 2023 15:57
@jules-ch
Copy link
Contributor Author

CI is failing because of a problem on CircleCI I think. Pipeline needs to be triggered again.

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.

query_api.query_raw does not return str but urllib3.response.HTTPResponse object
5 participants