IBM / nodejs-idb-connector

A JavaScript (Node.js) library for communicating with Db2 for IBM i, with support for queries, procedures, and much more. Uses traditional callback-style syntax
MIT License
37 stars 23 forks source link

[dbstmt] return numeric properties when sql types are numeric (#64) #77

Closed dmabupt closed 5 years ago

dmabupt commented 5 years ago

Related issue #64

jasonclake commented 5 years ago

Looking forward to this PR landing -- Thanks so much!

abmusse commented 5 years ago

I think we need to add tests for the new functionality to ensure it works properly.

jasonclake commented 5 years ago

@dmabupt, @abmusse I agree about the tests. What do you guys think about the follwing: https://github.com/jasonclake/nodejs-idb-connector/commit/3958dd115c52d352b369bbd900f91de1f4eace80

abmusse commented 5 years ago

@jasonclake

Nice work on the test cases!

I ran the test cases with the latest code on the numericData branch.

The first test case failed because of a difference in argument checking in asNumber function.

On your branch you allow for 0 or 1 parameters whereas on the numericData branch 1 parameter must be passed:

  dbstmt asNumber Flag
    1) should default to false
    ✓ when false should return numbers as strings (64ms)
    ✓ when true should return numbers when safe to do so (58ms)

  2 passing (140ms)
  1 failing

  1) dbstmt asNumber Flag
       should default to false:
     Error: SQLSTATE=PAERR SQLCODE=8001 asNumber() Expected One Parameter
      at Context.<anonymous> (test/statement.asNumber.js:13:25)
      at processImmediate (internal/timers.js:439:21)
jasonclake commented 5 years ago

Thanks. The first test can be removed if needed. It just seemed like a good idea to verify the default is false as documented (and stays that way). After getting it to work I wasn't quite sure the proper way to share both the tests and the required modification (for the first test to pass) to the existing pull request. I am open to any direction you may be able to give.

abmusse commented 5 years ago

It just seemed like a good idea to verify the default is false as documented (and stays that way).

I agree, the issue is that current code in the numericData branch we simply do not have a way of returning the state of the flag.

I think we should return the current state of the flag when statement.asNumber() is called without parameters. Like you have done here.

After getting it to work I wasn't quite sure the proper way to share both the tests and the required modification (for the first test to pass) to the existing pull request. I am open to any direction you may be able to give.

You can make a PR with those changes to the numericData branch.

EDIT:

Although it may be simpler if either @dmabupt or myself commit those changes directly into the numericData branch.

I will commit those changes into this PR

jasonclake commented 5 years ago

@abmusse That works for me!

abmusse commented 5 years ago

@jasonclake @dmabupt

I pushed commits to the numericData branch.

I chose to add the asNumber test cases to statementTest.js

dmabupt commented 5 years ago

Thanks for your contributions to refine the code.

abmusse commented 5 years ago

resolves #64