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

fix(utils/utils_str): recognize '+' as a valid numeric sign #3778

Merged
merged 5 commits into from
Sep 23, 2024

Conversation

wwwfeng
Copy link
Contributor

@wwwfeng wwwfeng commented Sep 13, 2024

我觉得+号也应该作为数字类型识别,我看标准库的atoi 也有类似对于带+符号的数字处理
比如

func ParseInt(s string, base int, bitSize int) (i int64, err error) {
	 //...
	// Pick off leading sign.
	s0 := s
	neg := false
	if s[0] == '+' {
		s = s[1:]
	} else if s[0] == '-' {
		neg = true
		s = s[1:]
	}
      //...
  return
}

@wwwfeng wwwfeng changed the title fix(utils/utils_str): fix recognize '+' as a valid numeric sign fix(utils/utils_str):#3776 fix recognize '+' as a valid numeric sign Sep 13, 2024
internal/utils/utils_str.go Show resolved Hide resolved
@gqcn
Copy link
Member

gqcn commented Sep 19, 2024

fixed #3776

@gqcn gqcn changed the title fix(utils/utils_str):#3776 fix recognize '+' as a valid numeric sign fix(utils/utils_str): recognize '+' as a valid numeric sign Sep 19, 2024
@wwwfeng
Copy link
Contributor Author

wwwfeng commented Sep 19, 2024

咦,CI的问题吗

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Hey, is it a CI problem?

@wln32
Copy link
Member

wln32 commented Sep 20, 2024

@wwwfeng @gqcn 按照如下更改是否会更好一些?

func IsNumeric(s string) bool {
	var (
		dotCount = 0
		length   = len(s)
		start    = 0
	)
	if length == 0 {
		return false
	}
	// -123 => 123
	// +123 => 123
	if s[0] == '-' || s[0] == '+' {
		// + false
		// - false
		if length == 1 {
			return false
		}
		start = 1
	}
	// .123 false
	// 123. false
	// .    false
	if s[0] == '.' || s[length-1] == '.' {
		return false
	}
	for i := start; i < length; i++ {
		if s[i] == '.' {
			dotCount++
			if dotCount > 1 {
				return false
			}
			continue
		}
		if s[i] < '0' || s[i] > '9' {
			return false
		}
	}
	return true
}

另再添加如下测试用例

t.Assert(utils.IsNumeric("123."), false)
		t.Assert(utils.IsNumeric(".123"), false)
		t.Assert(utils.IsNumeric("123.a"), false)
		t.Assert(utils.IsNumeric("a.123"), false)
		t.Assert(utils.IsNumeric("+"), false)
		t.Assert(utils.IsNumeric("-"), false)
		t.Assert(utils.IsNumeric("."), false)
                t.Assert(utils.IsNumeric("-."), false)
		t.Assert(utils.IsNumeric("+."), false)

@wwwfeng
Copy link
Contributor Author

wwwfeng commented Sep 21, 2024

@wwwfeng @gqcn 按照如下更改是否会更好一些?

func IsNumeric(s string) bool {
	var (
		dotCount = 0
		length   = len(s)
		start    = 0
	)
	if length == 0 {
		return false
	}
	// -123 => 123
	// +123 => 123
	if s[0] == '-' || s[0] == '+' {
		// + false
		// - false
		if length == 1 {
			return false
		}
		start = 1
	}
	// .123 false
	// 123. false
	// .    false
	if s[0] == '.' || s[length-1] == '.' {
		return false
	}
	for i := start; i < length; i++ {
		if s[i] == '.' {
			dotCount++
			if dotCount > 1 {
				return false
			}
			continue
		}
		if s[i] < '0' || s[i] > '9' {
			return false
		}
	}
	return true
}

另再添加如下测试用例

t.Assert(utils.IsNumeric("123."), false)
		t.Assert(utils.IsNumeric(".123"), false)
		t.Assert(utils.IsNumeric("123.a"), false)
		t.Assert(utils.IsNumeric("a.123"), false)
		t.Assert(utils.IsNumeric("+"), false)
		t.Assert(utils.IsNumeric("-"), false)
		t.Assert(utils.IsNumeric("."), false)
                t.Assert(utils.IsNumeric("-."), false)
		t.Assert(utils.IsNumeric("+."), false)

确实遗漏了一些case,似乎满足这些用例下面的变更就可以实现

if (s[i] == '-' || s[i] == '+') && i == 0 {
			if length == 1 {
				return false
			}
			continue
		}

// .123 false
// 123. false
这两个case原本处理逻辑似乎是包含了的

Copy link

sonarcloud bot commented Sep 23, 2024

@gqcn gqcn merged commit d8e3e9d into gogf:master Sep 23, 2024
29 checks passed
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.

4 participants