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

bugfix:add cases.NoLower option #7052

Merged
merged 4 commits into from
Sep 7, 2022

Conversation

wujunwei
Copy link
Contributor

@wujunwei wujunwei commented Sep 5, 2022

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

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:

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 08:19
@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.

Thanks @wujunwei. Can you please add a small test to lock this behaviour?

@wujunwei
Copy link
Contributor Author

wujunwei commented Sep 5, 2022

Thanks @wujunwei. Can you please add a small test to lock this behaviour?

I 'd like to do that , something like this ?

        source := "testAPI"
        assert.Equal(t,cases.Title(language.Und, cases.NoLower).String(source), strings.Title(source))
        assert.NotEqual(t,cases.Title(language.Und).String(source), strings.Title(source))

@kavirajk
Copy link
Contributor

kavirajk commented Sep 5, 2022

I 'd like to do that , something like this ?
source := "testAPI"
assert.Equal(t,cases.Title(language.Und, cases.NoLower).String(source), strings.Title(source))
assert.NotEqual(t,cases.Title(language.Und).String(source), strings.Title(source))

No. not really.

I mean we should test how we use it in Loki pkg/util/cfg/flag.go

Can you add a test case in flag_test.go?

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Sep 5, 2022
@wujunwei
Copy link
Contributor Author

wujunwei commented Sep 5, 2022

No. not really.

I mean we should test how we use it in Loki pkg/util/cfg/flag.go

Can you add a test case in flag_test.go?

Thanks for explaining. I have add some test case , Please take a look :)

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.

Thanks @wujunwei for addressing the feedback. LGTM 👍

Looks like CI is failing for some unrelated reasons. Can you please rebase your branch to main? I will merge it then.

@wujunwei wujunwei force-pushed the fix-deprecated-function-title branch from b59f5fd to 83ef0af Compare September 7, 2022 07:30
@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%

@wujunwei
Copy link
Contributor Author

wujunwei commented Sep 7, 2022

Have done so :) .

@kavirajk kavirajk merged commit eaf7f34 into grafana:main Sep 7, 2022
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.

3 participants