-
Notifications
You must be signed in to change notification settings - Fork 102
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
feat!: throw error with out of bounds integer values instead of truncating, add custom 'integerValue' type cast option #488
Closed
Closed
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
60c0900
feat: handle outside of bounds numeric values, add custom 'integerVal…
AVaksman 6d1c7cc
chore: add unit tests
AVaksman edeec3e
Merge branch 'master' into numeric_typeCast
AVaksman e882977
feat: add validation agains MIN_SAFE_INTEGER
AVaksman 5863365
refactor: remove extra condition
AVaksman 9358b68
Merge branch 'master' into numeric_typeCast
AVaksman a7856d8
Merge branch 'master' into numeric_typeCast
AVaksman 5f5be01
Merge branch 'master' into numeric_typeCast
AVaksman 8668bd5
refactor: more specific name
AVaksman e5b8635
Merge branch 'master' into numeric_typeCast
AVaksman d66115b
Merge branch 'master' into numeric_typeCast
JustinBeckwith 40f2f90
Merge branch 'master' into numeric_typeCast
AVaksman 4e6f530
refactor: use Number.isSafeInteger
AVaksman b856dc8
fix: docs
AVaksman e201f6e
fix: docs2
AVaksman 4c2ba71
Merge branch 'master' into numeric_typeCast
AVaksman 1366a7a
Merge branch 'master' into numeric_typeCast
AVaksman 4fb2306
Merge branch 'master' into numeric_typeCast
AVaksman 6770518
Merge branch 'master' into numeric_typeCast
AVaksman 68ce578
Merge branch 'master' into numeric_typeCast
jkwlui 7aa3617
Merge branch 'master' into numeric_typeCast
AVaksman 4b7cb3e
Merge branch 'master' into numeric_typeCast
JustinBeckwith File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is definitely a more robust option, but was just returning the DS.Int type considered?
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, I mixed up Datastore's "Int" with Spanner's: https://github.com/googleapis/nodejs-spanner/blob/f8ad0ece28e99dd9c6d7e985de56bd1b376f4712/src/codec.ts#L139 -- for simplicity's sake, would it be worth considering using that format instead? When a value is out of bounds, it will throw. The value is accessible via the "value" property in string form.
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.
will need to
try=>catch
forvalueOf
and iferror
still pass thevalue
to customtypeCastFunction
.integerValue
.valueOf
on default throw in case of out of bounds.valueOf
will return the custom function result.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.
WDYT?
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 if I misunderstand. Neither of the solutions make an extra network call, do they?
I think the throwing behavior is identical to what is in this PR. It will first try to just use it as a number, but if it's out of bounds when called upon, it throws. Or, is it because the new "Int" wrapper isn't technically a
Number
? Lots of gray area in determining a breaking change 😕I quite like that approach! Maybe @callmehiphop has a thought on this, too.
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 for clearing that up for me! For the Number vs customer "Int" type issue, they can at least be used interchangeably, the only difference would be
typeof
checks for Number wouldn't pass... unless Int extended Number? Curious to hear other thoughts on the best way to go forward.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.
Did we ever make progress on this, maybe in another chat?
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.
Quick summary
Would like to hear the thoughts on the approach
Integer
similar to Spanner, where#valueOf
would try to convert toNumber
and throw if out of bounds#value
returnsstring
representation of the valueInteger
object#valueOf
#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.
My vote would go for #3. Ping to @callmehiphop and @bcoe for more votes.
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.
My vote is starting simple with option 1, as a breaking change, and we can move towards an object representation in the future, if there's good reason.