reverse Timestamp() param order COMPASS-5733 #109
Conversation
| "@types/estree": "^0.0.41", | ||
| "@types/jest": "^26.0.21", | ||
| "benchmark": "^2.1.4", | ||
| "bson": "^4.2.3", | ||
| "bson": "^4.6.3", |
There was a problem hiding this comment.
Had to bump bson so we can gain support for {t, i } in tests. See mongodb/js-bson#449
| "np": "^7.4.0", | ||
| "prettier": "^1.19.1", | ||
| "ts-jest": "^26.5.4", | ||
| "typescript": "^3.9.9" | ||
| "typescript": "^4.3.5" |
There was a problem hiding this comment.
Had to bump typescript because bson became typescript and it was still expecting a @types/bson which kept giving me this error:
test/index.spec.ts:1:23 - error TS7016: Could not find a declaration file for module 'bson'. '/Users/leroux.bodenstein/mongo/ejson-shell-parser/node_modules/bson/lib/bson.js' implicitly has an 'any' type.
Try `npm install @types/bson` if it exists or add a new declaration (.d.ts) file containing `declare module 'bson';`
1 import * as bson from 'bson';
| @@ -49,6 +49,8 @@ it('should accept a complex query', function() { | |||
| ObjectId: ObjectId("5e159ba7eac34211f2252aaa"), | |||
| Symbol: Symbol('symbol'), | |||
| Timestamp: Timestamp(123, 456), | |||
| Timestamp_object: Timestamp({ t: 1, i: 2 }), | |||
| Timestamp_long: Timestamp(new Long(1, 2)), | |||
There was a problem hiding this comment.
Just trying to come up with a test case that shows that Timestamp()'s params aren't always two numbers.
| @@ -64,6 +64,13 @@ const SCOPE: { [x: string]: Function } = { | |||
| return new (bson as any).BSONSymbol(i); | |||
| }, | |||
| Timestamp: function(low: any, high: any) { | |||
| if (typeof low === 'number' && typeof high === 'number') { | |||
There was a problem hiding this comment.
Only nit is possibly we need to change the parameters from low to high to something easier to read/understand.
There was a problem hiding this comment.
I went back and forth on that. My current thinking (and really this is still very easy to change) is that the function gets the 32-bit words in that order. BUT Timestamp is overloaded and can take either one or two params. So in a sense it is just an args array that we have to inspect and either pass through unchanged or do something specific with in this one case.
Which is kinda the long way of saying I can't think of anything better than maybe ...args: any[].
There was a problem hiding this comment.
Maybe that's what we should do? I agree that it probably doesn't matter much either way, we're just passing stuff in and praying it's valid anyway.
There was a problem hiding this comment.
There was a problem hiding this comment.
@addaleax what do you think?
There was a problem hiding this comment.
No strong preferences, I think it’s fine to keep them as they are or to switch them, the important thing is that it’s visible to readers that something is going on here and I think that is being achieved :)
There was a problem hiding this comment.
Was about to suggest calling the params a and b, but that felt about as ridiculous, so I'm going to just merge :)
Compass should accept Timestamp(t, i), not Timestamp(i, t).
See