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

feat: add queryRows function #209

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Dec 20, 2024

Closes #33

Proposed Changes

Add queryRows, which return row as a Map

Checklist

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

@NguyenHoangSon96 NguyenHoangSon96 self-assigned this Dec 20, 2024
@NguyenHoangSon96 NguyenHoangSon96 linked an issue Dec 20, 2024 that may be closed by this pull request
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (8d6d3e8) to head (1f5421e).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...nfluxdb/v3/client/internal/InfluxDBClientImpl.java 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #209      +/-   ##
==========================================
+ Coverage   89.71%   89.78%   +0.06%     
==========================================
  Files          18       18              
  Lines         963      979      +16     
  Branches      150      151       +1     
==========================================
+ Hits          864      879      +15     
- Misses         39       40       +1     
  Partials       60       60              

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

@NguyenHoangSon96
Copy link
Contributor Author

@bednar
I ignored test cases for 2 lines. I think it's unnecessary
But please review and tell me if It's needed

@bednar bednar requested review from vlastahajek and karel-rehor and removed request for bednar January 7, 2025 09:27
Copy link
Contributor

@karel-rehor karel-rehor left a comment

Choose a reason for hiding this comment

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

I've added comments requesting a few changes. The ones that I might insist upon are related to examples in javadoc comments, which seem to not show the method being documented. or otherwise appear to be broken.

It would be nice to extend testing.

  • One or two unit tests for new methods would be nice
  • The integration test could be more complete
  • When testing integration it would be nice to test a couple of basic handled exceptional situations.

@NguyenHoangSon96
Copy link
Contributor Author

@karel-rehor
The issues in the documents were my stupid mistakes; I forgot to edit the comment after copying it from query() function 😂
I edited the code, please check again
Thank you

Copy link
Contributor

@karel-rehor karel-rehor left a comment

Choose a reason for hiding this comment

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

There are still some issues with the Javadoc examples. The arguments in many of the examples do not match the method they are documenting. Also, when looking at how the javadoc is rendered in Intellij, the formatting of the examples could be improved to be more easily readable.

@NguyenHoangSon96
Copy link
Contributor Author

Hi @karel-rehor
I fixed the comments
Please check 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.

Add query method returning enumerator over rows
2 participants