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

Deprecation Fix: Replace strings.Title with cases.Title().String() #7048

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

wujunwei
Copy link
Contributor

@wujunwei wujunwei commented Sep 5, 2022

Signed-off-by: wujunwei [email protected]

What this PR does / why we need it:

Golang 1.18 deprecates the strings.title function use cases.Title(language.Und, cases.NoLower).String() instead.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Is this an important fix or new feature? Add an entry in the CHANGELOG.md.
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

@wujunwei wujunwei requested a review from a team as a code owner September 5, 2022 06:07
@CLAassistant
Copy link

CLAassistant commented Sep 5, 2022

CLA assistant check)
All committers have signed the CLA.

Signed-off-by: wujw39640 <[email protected]>
@grafanabot
Copy link
Collaborator

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

Copy link
Contributor

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @wujunwei .

Double checked with go1.18 doc.
https://pkg.go.dev/[email protected]#Title

Also surprised we use it just in one place. :)

loki$ rg "strings.Title" -g '!vendor'
pkg/util/cfg/flag.go
94:			fmt.Fprintf(fs.Output(), " %s:\n", strings.Title(name))

@kavirajk kavirajk merged commit d69d3b6 into grafana:main Sep 5, 2022
@wujunwei
Copy link
Contributor Author

wujunwei commented Sep 5, 2022

LGTM. Thanks @wujunwei .

Double checked with go1.18 doc. https://pkg.go.dev/[email protected]#Title

Also surprised we use it just in one place. :)

loki$ rg "strings.Title" -g '!vendor'
pkg/util/cfg/flag.go
94:			fmt.Fprintf(fs.Output(), " %s:\n", strings.Title(name))

I'm terribly sorry about this PR,I found I forget to add cases.NoLower option, if we don't apply cases.NoLower, it will upper first alphabet lower the others which have different effect from strings.title. Sorry for that again.

@wujunwei wujunwei mentioned this pull request Sep 5, 2022
4 tasks
@wujunwei
Copy link
Contributor Author

wujunwei commented Sep 5, 2022

@kavirajk I have offered a new PR to fix this. PTAL .

kavirajk pushed a commit that referenced this pull request Sep 7, 2022
<!--  Thanks for sending a pull request!  Before submitting:

1. Read our CONTRIBUTING.md guide
2. Name your PR as `<Feature Area>: Describe your change`.
a. Do not end the title with punctuation. It will be added in the
changelog.
b. Start with an imperative verb. Example: Fix the latency between
System A and System B.
  c. Use sentence case, not title case.
d. Use a complete phrase or sentence. The PR title will appear in a
changelog, so help other people understand what your change will be.
3. Rebase your PR if it gets out of sync with main
-->

**What this PR does / why we need it**:
see #7048 
if we don't apply cases.NoLower, it will upper first alphabet lower the
others
```go
package main

import (
	"fmt"
	"golang.org/x/text/cases"
	"golang.org/x/text/language"
	"strings"
)

func main() {
	source := "testAPI"
	fmt.Println(cases.Title(language.Und).String(source)) // out: Testapi
	fmt.Println(cases.Title(language.Und, cases.NoLower).String(source)) // out: TestAPI
	fmt.Println(strings.Title(source)) // out: TestAPI
}
```
**Which issue(s) this PR fixes**:


**Special notes for your reviewer**:

<!--
Note about CHANGELOG entries, if a change adds:
* an important feature
* fixes an issue present in a previous release, 
* causes a change in operation that would be useful for an operator of
Loki to know
then please add a CHANGELOG entry.

For documentation changes, build changes, simple fixes etc please skip
this step. We are attempting to curate a changelog of the most relevant
and important changes to be easier to ingest by end users of Loki.

Note about the upgrade guide, if this changes:
* default configuration values
* metric names or label names
* changes existing log lines such as the metrics.go query output line
* configuration parameters 
* anything to do with any API
* any other change that would require special attention or extra steps
to upgrade
Please document clearly what changed AND what needs to be done in the
upgrade guide.
-->
**Checklist**
- [ ] Documentation added
- [ ] Tests updated
- [ ] Is this an important fix or new feature? Add an entry in the
`CHANGELOG.md`.
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`

Signed-off-by: wujw39640 <[email protected]>
Signed-off-by: wujunwei <[email protected]>
Co-authored-by: wujw39640 <[email protected]>
lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022
…rafana#7048)

Signed-off-by: wujunwei <[email protected]>

<!--  Thanks for sending a pull request!  Before submitting:

1. Read our CONTRIBUTING.md guide
2. Name your PR as `<Feature Area>: Describe your change`.
  a. Do not end the title with punctuation. It will be added in the changelog.
  b. Start with an imperative verb. Example: Fix the latency between System A and System B.
  c. Use sentence case, not title case.
  d. Use a complete phrase or sentence. The PR title will appear in a changelog, so help other people understand what your change will be.
3. Rebase your PR if it gets out of sync with main
-->

**What this PR does / why we need it**:

[Golang 1.18 deprecates the `strings.title function`](golang/go#48367)  use `cases.Title(language.Und, cases.NoLower).String()` instead.

**Which issue(s) this PR fixes**:


**Special notes for your reviewer**:

<!--
Note about CHANGELOG entries, if a change adds:
* an important feature
* fixes an issue present in a previous release, 
* causes a change in operation that would be useful for an operator of Loki to know
then please add a CHANGELOG entry.

For documentation changes, build changes, simple fixes etc please skip this step. We are attempting to curate a changelog of the most relevant and important changes to be easier to ingest by end users of Loki.

Note about the upgrade guide, if this changes:
* default configuration values
* metric names or label names
* changes existing log lines such as the metrics.go query output line
* configuration parameters 
* anything to do with any API
* any other change that would require special attention or extra steps to upgrade
Please document clearly what changed AND what needs to be done in the upgrade guide.
-->
**Checklist**
- [ ] Documentation added
- [ ] Tests updated
- [ ] Is this an important fix or new feature? Add an entry in the `CHANGELOG.md`.
- [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`

Signed-off-by: wujw39640 <[email protected]>
lxwzy pushed a commit to lxwzy/loki that referenced this pull request Nov 7, 2022
<!--  Thanks for sending a pull request!  Before submitting:

1. Read our CONTRIBUTING.md guide
2. Name your PR as `<Feature Area>: Describe your change`.
a. Do not end the title with punctuation. It will be added in the
changelog.
b. Start with an imperative verb. Example: Fix the latency between
System A and System B.
  c. Use sentence case, not title case.
d. Use a complete phrase or sentence. The PR title will appear in a
changelog, so help other people understand what your change will be.
3. Rebase your PR if it gets out of sync with main
-->

**What this PR does / why we need it**:
see grafana#7048 
if we don't apply cases.NoLower, it will upper first alphabet lower the
others
```go
package main

import (
	"fmt"
	"golang.org/x/text/cases"
	"golang.org/x/text/language"
	"strings"
)

func main() {
	source := "testAPI"
	fmt.Println(cases.Title(language.Und).String(source)) // out: Testapi
	fmt.Println(cases.Title(language.Und, cases.NoLower).String(source)) // out: TestAPI
	fmt.Println(strings.Title(source)) // out: TestAPI
}
```
**Which issue(s) this PR fixes**:


**Special notes for your reviewer**:

<!--
Note about CHANGELOG entries, if a change adds:
* an important feature
* fixes an issue present in a previous release, 
* causes a change in operation that would be useful for an operator of
Loki to know
then please add a CHANGELOG entry.

For documentation changes, build changes, simple fixes etc please skip
this step. We are attempting to curate a changelog of the most relevant
and important changes to be easier to ingest by end users of Loki.

Note about the upgrade guide, if this changes:
* default configuration values
* metric names or label names
* changes existing log lines such as the metrics.go query output line
* configuration parameters 
* anything to do with any API
* any other change that would require special attention or extra steps
to upgrade
Please document clearly what changed AND what needs to be done in the
upgrade guide.
-->
**Checklist**
- [ ] Documentation added
- [ ] Tests updated
- [ ] Is this an important fix or new feature? Add an entry in the
`CHANGELOG.md`.
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/upgrading/_index.md`

Signed-off-by: wujw39640 <[email protected]>
Signed-off-by: wujunwei <[email protected]>
Co-authored-by: wujw39640 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants