Skip to content
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

Quote the numbers even if they are out of range #593

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Conversation

shuheiktgw
Copy link
Collaborator

Closes #586

The IsNeedQuoted function previously returned false when a given string was out of range, as ToNumber failed to parse such strings. I updated the IsNeedQuoted function to check the error returned by ToNumber and, if it is strconv.ErrRange, ensure the string is quoted instead.

Before submitting your PR, please confirm the following.

  • Describe the purpose for which you created this PR.
  • Create test code that corresponds to the modification

@shuheiktgw shuheiktgw requested a review from goccy December 15, 2024 17:58
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.47%. Comparing base (e1d8782) to head (3e2fe49).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #593      +/-   ##
==========================================
+ Coverage   77.43%   77.47%   +0.04%     
==========================================
  Files          21       21              
  Lines        7772     7787      +15     
==========================================
+ Hits         6018     6033      +15     
  Misses       1342     1342              
  Partials      412      412              

ast/ast.go Outdated
@@ -342,7 +342,7 @@ func Bool(tk *token.Token) *BoolNode {
// Integer create node for integer value
func Integer(tk *token.Token) *IntegerNode {
var v any
if num := token.ToNumber(tk.Value); num != nil {
if num, _ := token.ToNumber(tk.Value); num != nil {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd

Shouldn't it be better and more logic now you refactored to have something like that

Suggested change
if num, _ := token.ToNumber(tk.Value); num != nil {
if num, err := token.ToNumber(tk.Value); err == nil {

With code refactored to return an error when

token/token.go Outdated
Comment on lines 648 to 654
if err != nil {
// Need quotes even when the value is out of range
var numErr *strconv.NumError
if errors.As(err, &numErr) && errors.Is(numErr.Err, strconv.ErrRange) {
return true
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is enough

Suggested change
if err != nil {
// Need quotes even when the value is out of range
var numErr *strconv.NumError
if errors.As(err, &numErr) && errors.Is(numErr.Err, strconv.ErrRange) {
return true
}
}
// Need quotes even when the value is out of range
var numErr *strconv.NumError
if errors.As(err, &numErr) && errors.Is(numErr.Err, strconv.ErrRange) {
return true
}

token/token.go Outdated
@@ -538,16 +539,16 @@ type NumberValue struct {
Text string
}

func ToNumber(value string) *NumberValue {
func ToNumber(value string) (*NumberValue, error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks like a hammer to fix a scratch.

You are updating the signature of an exported method, it would have consequence.

I would have checked the ErrRange error in this method, and set a boolean variable NeedQuote on the NumberValue struct, then use it in IsQuoteNeeded

Copy link
Collaborator Author

@shuheiktgw shuheiktgw Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your comments, @ccoVeille! Hmm, you are right. I haven't thought about the possibility that people use the method, but let me think about it a little more carefully 👍

@shuheiktgw
Copy link
Collaborator Author

I’ve migrated most of the logic from ToNumber to a new function called toNumber, preserved the original interface of ToNumber, and created a new function called isNumber to check if a given string is a number 🙂

@goccy goccy merged commit 61bc6c1 into master Dec 16, 2024
19 checks passed
@goccy goccy deleted the fix_out_of_range branch December 16, 2024 09:53
@ccoVeille
Copy link

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large hex strings are unquoted when marshalled
4 participants