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

Make sql execute return promise api-877 #1046

Merged
merged 9 commits into from Sep 24, 2021
Merged

Conversation

srknzl
Copy link
Member

@srknzl srknzl commented Sep 14, 2021

Makes execute and executeStatement methods in SqlService return promises of SqlResult. Update tests, code samples and documentation accordingly.

Note that we do not need any conditional tests for back compatibility because using await with a non async function works. It will convert the returned value to a promise like Promise.resolve(returnedValue) implicitly but I think it is better not to change tests for readability. The promise overhead only affects ~50 tests and the overhead is very small.

@@ -2,3 +2,4 @@ src/codec/*.ts
src/codec/custom/*.ts
src/.eslintrc.js
lib/**
docs/**
Copy link
Member Author

@srknzl srknzl Sep 14, 2021

Choose a reason for hiding this comment

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

Needed because of linting does eslint .. I forgot to add docs folder before

@srknzl srknzl changed the title Make sql execute return promise Make sql execute return promise api-877 Sep 14, 2021
@@ -483,22 +482,22 @@ describe('Data type test', function () {
`
var map = instance_0.getMap("${mapName}");
for (var key = 1; key < 12; key++) {
map.set(new java.lang.Integer(key), java.time.LocalDate.of(key+50002,key+1,key));
map.set(new java.lang.Integer(key), java.time.LocalDate.of(key+5002,key+1,key));
Copy link
Member Author

@srknzl srknzl Sep 14, 2021

Choose a reason for hiding this comment

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

I changed this for back compat tests because year was a short in 4.2.

@codecov
Copy link

codecov bot commented Sep 14, 2021

Codecov Report

Merging #1046 (fa4ed9a) into master (e07ef8d) will decrease coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1046      +/-   ##
==========================================
- Coverage   92.56%   92.32%   -0.25%     
==========================================
  Files         432      432              
  Lines       16730    16712      -18     
  Branches     1037     1034       -3     
==========================================
- Hits        15486    15429      -57     
- Misses        979     1017      +38     
- Partials      265      266       +1     
Impacted Files Coverage Δ
src/sql/SqlResult.ts 96.07% <100.00%> (+2.58%) ⬆️
src/sql/SqlService.ts 94.69% <100.00%> (+1.96%) ⬆️
src/network/ConnectionManager.ts 81.12% <0.00%> (-6.18%) ⬇️
src/config/ConfigBuilder.ts 93.95% <0.00%> (-2.85%) ⬇️
src/invocation/ClusterService.ts 93.61% <0.00%> (-0.71%) ⬇️
src/util/Util.ts 87.74% <0.00%> (-0.65%) ⬇️
src/proxy/MultiMapProxy.ts 97.14% <0.00%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e07ef8d...fa4ed9a. Read the comment docs.

src/sql/SqlService.ts Show resolved Hide resolved
src/sql/SqlService.ts Show resolved Hide resolved
code_samples/sql-data-types.js Outdated Show resolved Hide resolved
src/sql/SqlService.ts Show resolved Hide resolved
src/sql/SqlResult.ts Outdated Show resolved Hide resolved
src/sql/SqlResult.ts Show resolved Hide resolved
src/sql/SqlService.ts Show resolved Hide resolved
code_samples/sql-basic-usage.js Outdated Show resolved Hide resolved
code_samples/sql-data-types.js Outdated Show resolved Hide resolved
code_samples/sql-data-types.js Outdated Show resolved Hide resolved
code_samples/sql-data-types.js Outdated Show resolved Hide resolved
src/sql/SqlResult.ts Show resolved Hide resolved
src/sql/SqlService.ts Show resolved Hide resolved
@srknzl srknzl merged commit 500db27 into hazelcast:master Sep 24, 2021
harunalpak pushed a commit to harunalpak/hazelcast-nodejs-client that referenced this pull request Dec 8, 2022
* Make execute return promise

* Update code samples and docs

* Change date string format

* Change year

* Update SqlResult

* Update sql service

* Update tests

* Review comments

* Fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants