-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add String type #29
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bbernays, added a few comments
src/scalar/text.ts
Outdated
public toString = (): string => { | ||
if (this._valid) { | ||
return this._value.toString(); | ||
} | ||
|
||
return NULL_VALUE; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erezrokah - Do you know why I had to change this method from:
public toString() {
if (this._valid) {
return this._value.toString();
}
return NULL_VALUE;
};
In order for the Object.hasOwn
to see that this class has a toString
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is in https://stackoverflow.com/questions/22445261/why-does-hasownproperty-not-recognise-functions-on-an-objects-prototype
I think if we do typeof value!.toString === 'function' && value!.toString !== Object.prototype.toString
is should work for both cases
src/scalar/text.ts
Outdated
|
||
switch (typeof value) { | ||
case 'object': { | ||
if (value !== undefined && value !== null && Object.hasOwn(value, 'toString')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (value !== undefined && value !== null && Object.hasOwn(value, 'toString')) { | |
if (Object.hasOwn(value, 'toString')) { |
We already check for invalid value
above with isInvalid()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this check the typescript compiler/builder gave errors. If there is a way around that I would be happy to change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do value!
to tell TypeScript is can never be null
or undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, did a review yesterday but forgot to submit it 🤦
src/scalar/text.ts
Outdated
public toString = (): string => { | ||
if (this._valid) { | ||
return this._value.toString(); | ||
} | ||
|
||
return NULL_VALUE; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is in https://stackoverflow.com/questions/22445261/why-does-hasownproperty-not-recognise-functions-on-an-objects-prototype
I think if we do typeof value!.toString === 'function' && value!.toString !== Object.prototype.toString
is should work for both cases
Going to merge this as I need this scalar for the sync |
🤖 I have created a release *beep* *boop* --- ## [0.0.3](v0.0.2...v0.0.3) (2023-08-16) ### Features * Add `addCQIDsColumns` util, split MemDB into files ([#51](#51)) ([dd71c60](dd71c60)) * Add arrow dependency ([#15](#15)) ([082213f](082213f)) * Add Bool scalar ([#17](#17)) ([80edcd5](80edcd5)) * Add CLI argument parsing ([#10](#10)) ([0d70b25](0d70b25)) * Add CQID and Parent CQID ([#53](#53)) ([fbb7c94](fbb7c94)) * Add initial sync, scheduler ([#37](#37)) ([9b41287](9b41287)) * Add JSON scalar ([#55](#55)) ([7e39769](7e39769)) * Add multiplexer, round-robin ([#48](#48)) ([00a842a](00a842a)) * Add String type ([#29](#29)) ([4c07a3d](4c07a3d)) * Add table types ([#32](#32)) ([c7b301d](c7b301d)) * Add UUID scalar ([#54](#54)) ([d8474ce](d8474ce)) * Add UUID, JSON types ([#56](#56)) ([45ab919](45ab919)) * Implement discovery service ([#19](#19)) ([3abe320](3abe320)) * Implement logger ([#21](#21)) ([43becf1](43becf1)) * List scalars ([#36](#36)) ([0eb6d62](0eb6d62)) * MemDB read ([#43](#43)) ([3429de0](3429de0)), closes [#41](#41) * MemDB writes ([#42](#42)) ([8f21f52](8f21f52)) * More scalars ([#50](#50)) ([a9589b6](a9589b6)) * Scaffold plugin server ([#27](#27)) ([f0864eb](f0864eb)) * **scalars:** Timestamp, float64, int64 ([#35](#35)) ([7fc391c](7fc391c)) * Transform objects ([#34](#34)) ([c14f017](c14f017)), closes [#5](#5) * Transformers ([#25](#25)) ([e96adb3](e96adb3)) ### Bug Fixes * @grpc/grpc-js dependency ([2adf06d](2adf06d)) * **deps:** Update dependency @cloudquery/plugin-pb-javascript to ^0.0.6 ([#14](#14)) ([92a87a1](92a87a1)) * Encode tables in migrate messages ([#38](#38)) ([f6413d2](f6413d2)) * Error handling, `null` values in scalars, proper exports ([#61](#61)) ([2b283d1](2b283d1)) * Extension types null values ([#58](#58)) ([e8bb0a0](e8bb0a0)) * Flatten tables in GetTables gRPC call ([#57](#57)) ([5a8f3c3](5a8f3c3)) * Implement sync, scheduler, resource encoding ([#44](#44)) ([4a5f9e8](4a5f9e8)) * Parent CQId resolver ([8de14cb](8de14cb)) * Properly encode uuid ([#59](#59)) ([72efa22](72efa22)) * Support only tcp network ([#23](#23)) ([e138e40](e138e40)) * Write gRPC call, use `for await` on write readble stream ([#52](#52)) ([773a0e5](773a0e5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
No description provided.