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

Replace use of strings.Title with cases.Title #2268

Merged
merged 11 commits into from
Jul 3, 2022

Conversation

tmc
Copy link
Contributor

@tmc tmc commented Jul 2, 2022

This removes the use of the deprecated strings.Title function and enables go 1.18 in CI while still working on Go 1.16+.

Integration Security Lint Test

Depends on #2266 (will rebase once that's in).

@tmc tmc changed the title Use cases go1.18: Replace use of strings.Title with cases.Title Jul 2, 2022
@coveralls
Copy link

coveralls commented Jul 2, 2022

Coverage Status

Coverage increased (+0.06%) to 75.075% when pulling 4f3aaac on tmc:use-cases into 0c11e5f on 99designs:master.

@tmc tmc marked this pull request as ready for review July 2, 2022 22:03
Copy link
Collaborator

@a8m a8m left a comment

Choose a reason for hiding this comment

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

Looks good to me. My only concern before merging is if we introduce any breaking change by moving from strings.Title to cases.Title. Can you check please and add a link here? Thanks

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jul 3, 2022

Thanks! This was reported to Go in 2013 in golang/go#6801.

I think that your usage of cases.Title(language.English, cases.NoLower) is pretty safe as NoLower avoids trampling on initialisms like those listed here. I think otherwise language.English matches the current behavior for now, and it opens the possibility of supporting multilingual in the future through some configuration without speculatively adding that complexity now. We can see if there's much demand for that in the future.

Given that, I'm not sure if adding further unit tests are necessary now, but as a note to our future selves, the two test cases I have used elsewhere for multilingual capitalization are dealing with unicode punctuation and language specific capitalization.

  1. Unicode punctuation can be tested like this:
import (
    "fmt"
    "strings"
)

func main() {
    a := strings.Title("go.go\u2024go")
    b := "Go.Go\u2024Go"
    if a != b {
        fmt.Printf("%s != %s\n", a, b) // Go.Go․go != Go.Go․Go
    }
}
  1. The language specific test case I've used is in Dutch words, "ijsland" should be capitalized as "IJsland", but the strings.Title("ijsland") is converted to "Ijsland".
caser := cases.Title(language.Dutch)
caser.String("ijsland") // Ijsland

@StevenACoffman
Copy link
Collaborator

golang.org/x/text/cases currently has a go.mod with a go directive that indicates that the module was written assuming the semantics of the Go version 1.17 as here https://cs.opensource.google/go/x/text/+/master:go.mod;l=10 but appears to work fine in Go 1.16 as all gqlgen tests still pass in Go 1.16.

@StevenACoffman StevenACoffman changed the title go1.18: Replace use of strings.Title with cases.Title Replace use of strings.Title with cases.Title Jul 3, 2022
@StevenACoffman StevenACoffman merged commit 0b0e5ce into 99designs:master Jul 3, 2022
@tmc tmc deleted the use-cases branch July 5, 2022 00:10
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