brianc / node-pg-types

Type parsing for node-postgres
267 stars 54 forks source link

Throw an error when oid is missing #129

Closed kapouer closed 3 years ago

kapouer commented 3 years ago

Fixes #128

kapouer commented 3 years ago

I'm not sure how to correctly use tape to test that the thrown error is TypeError

codecov-io commented 3 years ago

Codecov Report

Merging #129 (03b8699) into master (05badcd) will increase coverage by 1.52%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   91.74%   93.26%   +1.52%     
==========================================
  Files           4        4              
  Lines         206      208       +2     
==========================================
+ Hits          189      194       +5     
+ Misses         17       14       -3     
Impacted Files Coverage Δ
index.js 90.90% <100.00%> (+15.90%) :arrow_up:

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 05badcd...03b8699. Read the comment docs.

sehrope commented 3 years ago

You can use a regex in the second arg of t.throws(...) to validate the thrown error:

t.test('setTypeParser should throw when oid is not an integer', function (t) {
    t.throws(function () {
      setTypeParser(null, function () {})
    }, /^TypeError: oid must be an integer/)    // <-- Add this
    t.throws(function () {
      setTypeParser('a', function () {})
    }, /^TypeError: oid must be an integer/)    // <-- Add this
    t.end()
  })
kapouer commented 3 years ago

Ha, ok. Done and force-pushed.

bendrucker commented 3 years ago

Perfect, thanks!

sehrope commented 3 years ago

LGTM.

@bendrucker Should add a note that the only thing this might break is someone setting a parser using the string representation of an int for the oid value. That would have worked before as the prop lookup of the oid gets stringified but I don't think it's a big deal to break that use case. A note should be fine as it's undocumented behavior and anyone would see it at app startup too.

bendrucker commented 3 years ago

Good catch! This is going to go out in a major release that's months overdue here anyway. Will make sure to note it in the main pg changelog.