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: respect iox::column_type::field metadata when mapping query #491

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

Conversation

NguyenHoangSon96
Copy link
Contributor

@NguyenHoangSon96 NguyenHoangSon96 commented Dec 17, 2024

Closes #

Proposed Changes

Briefly describe your proposed changes:

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 17, 2024
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

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

Project coverage is 97.70%. Comparing base (219985b) to head (643037c).

Files with missing lines Patch % Lines
packages/client/src/util/TypeCasting.ts 96.96% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #491   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          23       24    +1     
  Lines         914      960   +46     
  Branches      218      234   +16     
=======================================
+ Hits          893      938   +45     
- Misses         10       11    +1     
  Partials       11       11           

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

Copy link
Member

@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.

Upgrade the client version to 1.0.0:

export const CLIENT_LIB_VERSION = '0.12.0'

@bednar bednar requested review from bednar, vlastahajek and karel-rehor and removed request for bednar and vlastahajek December 17, 2024 07:46
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 have one question about a branch in util/TypeCasting.ts and two recommendations for improving tests.

Once the question is clarified this can be approved. It might be nice to try dynamic test generation in mocha for some of the new tests.

packages/client/test/unit/util/common.test.ts Show resolved Hide resolved
packages/client/src/util/TypeCasting.ts Outdated Show resolved Hide resolved
@NguyenHoangSon96
Copy link
Contributor Author

Hi @karel-rehor
Thank you for your review
I made some adjustments,
Please check when you have time

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.

Tests pass locally with nodejs v1.21. N.B. there was a failing test with nodejs v1.22, however it is unrelated to this PR. A note has been added to the backlog.

Changes make sense.

@bednar bednar requested a review from vlastahajek December 18, 2024 09:49
@NguyenHoangSon96
Copy link
Contributor Author

Tests pass locally with nodejs v1.21. N.B. there was a failing test with nodejs v1.22, however it is unrelated to this PR. A note has been added to the backlog.

Changes make sense.

Tests pass locally with nodejs v1.21. N.B. there was a failing test with nodejs v1.22, however it is unrelated to this PR. A note has been added to the backlog.

Changes make sense.

@karel-rehor
Thank you for your review
I made the PR for support node v22 here
#492

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