chore: expand inspect support of BSON types NODE-2875 #413
Conversation
src/binary.ts
Outdated
| // with a non-standard length. | ||
| // eslint-disable-next-line no-fallthrough | ||
| default: | ||
| return `BinData(${this.sub_type}, "${asBuffer.toString('base64')}")`; |
There was a problem hiding this comment.
@rose-m These are also shell types, so I think at least this one would probably be best changed as well (and, because the different names aren’t enough confusing, the arguments work pretty differently … BinData() in the shell takes subtype + base64 string in that order, Binary in BSON takes a “““binary””” string or buffer + subtype, i.e. in reverse order)… I think ideally this line could be something like
| return `BinData(${this.sub_type}, "${asBuffer.toString('base64')}")`; | |
| return `Binary(Buffer.from("${asBuffer.toString('base64')}", "base64"), ${this.sub_type})`; |
but it sure isn’t pretty…
(edit: if you want to know why I triple-quoted “binary”: https://twitter.com/addaleax/status/1189711048682680320)
test/node/symbol_tests.js
Outdated
| const util = require('util'); | ||
| const BSONSymbol = BSON.BSONSymbol; | ||
|
|
||
| describe('MinKey tests', function () { |
There was a problem hiding this comment.
| describe('MinKey tests', function () { | |
| describe('BSONSymbol tests', function () { |
There was a problem hiding this comment.
oops, missed this one. Actually, now that we're here does it make sense to just add all of these tests in bson_tests instead? I'm imagining a future maintainer forgetting to change one of these in the future because they have to change ~10 files
There was a problem hiding this comment.
Yeah I wasn't sure how to handle that and then went with the individual files. I could implement a test case like https://github.com/mongodb/js-bson/blob/master/test/node/bson_test.js#L1310 which would inspect an object with all the different types and directly compare that instead of individual test cases?
There was a problem hiding this comment.
I like the idea of checking the behavior in one place, it looks like maybe there's a build failure related to formatting of the string. Maybe its easier to test each type in isolation, rather than inspecting an object with each type? We've had spurious build failures in the past due to minor fluctuations in formatting.
There was a problem hiding this comment.
Ah alright now I see - sometimes it has line breaks, sometimes it doesn't. Will fix asap and split it up into individual cases to circumvent that.
There was a problem hiding this comment.
I've now added individual cases per type in one block in bson_tests - let's hope these now work in evergreen 😄
rose-m
force-pushed
the
NODE-2875-expand-bson-type-inspect-support
branch
2 times, most recently
from
6bb1855
to
2110bae
Compare
rose-m
force-pushed
the
NODE-2875-expand-bson-type-inspect-support
branch
from
2110bae
to
b309c22
Compare
Description
Adds support for the NodeJS
inspectfunction to more BSON data types. This is an upstream change proposed by the mongosh team.A custom
inspectfunction is added for the following data types:BinaryCodeDBRefDecimal128Int32LongMinKeyMaxKeyBSONSymbolTimestamp