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

Use double quotes consistently in en-US #24141

Merged
merged 3 commits into from
Apr 17, 2023

Conversation

n0toose
Copy link
Contributor

@n0toose n0toose commented Apr 15, 2023

Also removes quotes in commit messages related to file modifications
made in the Web UI.

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Used single quotes, as there wasn't a clear policy around this and I only managed to find the following PR mentioning something along the lines of "we should use single quotes to remain consistent": #3512 (comment)

This issue was implemented after I started implementing a small UX change after some feedback: https://codeberg.org/Codeberg/Community/issues/991

I wasn't sure whether to use single or double quotes, so I just went for it. Some languages (e.g. French) use double quotes much more often, but most of them are fairly mixed up. If using double quotes in specific locales was a conscious decision, I find that it can be fixed later, and what what I did is a better alternative than "destroying" preexisting translations and requiring a lot of manual input from the translators themselves.

If that approach happens to be misguided, please do let me know, and I'll take any appropriate steps to fix it.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 15, 2023
@silverwind
Copy link
Member

silverwind commented Apr 15, 2023

To me, double quotes seem more natural, but from what I gather, it's primarly US (which prefers double), vs. UK (which prefers single) english. Other languages may have other preferences.

Also, if we are already doing quote normalization, we should replace the typographic ones „“ as well, and ideally add their replacement into a sed call in make fmt to enforce one style in CI.

@silverwind
Copy link
Member

en-US file should use double quotes. All other files are to be handled on Crowdin, and I don't think we need to modify them here.

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Also, if we are already doing quote normalization, we should replace the typographic ones „“ as well, and ideally add their replacement into a sed call in make fmt to enforce one style in CI.

Forgot about German, maybe enforcing something that is very """Anglospheric""" in the first place was a bad idea. I'll "remake" this PR real quick and update it here accordingly.

@n0toose n0toose changed the title Use single quotes consistently Use double quotes consistently in en-US Apr 15, 2023
@silverwind
Copy link
Member

I suppose if a language has a different quote preference, they should just do it during the translation process, so make this PR only touch the en-US file, which as per above should be double quoted. I think a simple sed call to replace single+typhograhic quotes in that file should be sufficient.

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Even if it would work here, I tend to avoid sed in changes like this because of edge cases such as, namely, a command that was included in the translation files.

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Upon manual review, I realized that I missed some other edge cases, correcting...

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Fixing tests...

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Note to self

Fix this thing (and two other similar errors) here later.

=== TestViewRepo2 (/home/user/gitea/tests/integration/repo_test.go:46)
--- FAIL: TestViewRepo2 (0.29s)
    testlogger.go:77: 2023/04/15 19:52:20 ...les/storage/local.go:47:NewLocalStorage() [I] Creating new Local Storage at /home/user/gitea/tests/gitea-lfs-meta
    testlogger.go:77: 2023/04/15 19:52:20 ...eb/routing/logger.go:98:func1() [I] [643ae454-120] router: completed GET /user/login for , 200 OK in 1.3ms @ auth/auth.go:141(auth.SignIn)
    testlogger.go:77: 2023/04/15 19:52:20 ...eb/routing/logger.go:98:func1() [I] [643ae454-121] router: completed POST /user/login for , 303 See Other in 5.0ms @ auth/auth.go:170(auth.SignInPost)
    testlogger.go:77: 2023/04/15 19:52:20 ...eb/routing/logger.go:98:func1() [I] [643ae454-122] router: completed GET /user3/repo3 for , 200 OK in 13.5ms @ repo/view.go:707(repo.Home)
    repo_test.go:86: 
                Error Trace:    /home/user/gitea/repo_test.go:86
                                                        /home/user/gitea/repo_test.go:104
                Error:          Not equal: 
                                expected: []integration.file{integration.file{fileName:"doc", commitID:"2a47ca4b614a9f5a43abbd5ad851a54a616ffee6", commitMsg:"init project", commitTime:"Wed, 14 Jun 2017 15:54:21 CEST"}, integration.file{fileName:"README.md", commitID:"2a47ca4b614a9f5a43abbd5ad851a54a616ffee6", commitMsg:"init project", commitTime:"Wed, 14 Jun 2017 15:54:21 CEST"}}
                                actual  : []integration.file{integration.file{fileName:"doc", commitID:"2a47ca4b614a9f5a43abbd5ad851a54a616ffee6", commitMsg:"init project", commitTime:"Wed, 14 Jun 2017 13:54:21 UTC"}, integration.file{fileName:"README.md", commitID:"2a47ca4b614a9f5a43abbd5ad851a54a616ffee6", commitMsg:"init project", commitTime:"Wed, 14 Jun 2017 13:54:21 UTC"}}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -5,3 +5,3 @@
                                   commitMsg: (string) (len=12) "init project",
                                -  commitTime: (string) (len=30) "Wed, 14 Jun 2017 15:54:21 CEST"
                                +  commitTime: (string) (len=29) "Wed, 14 Jun 2017 13:54:21 UTC"
                                  },
                                @@ -11,3 +11,3 @@
                                   commitMsg: (string) (len=12) "init project",
                                -  commitTime: (string) (len=30) "Wed, 14 Jun 2017 15:54:21 CEST"
                                +  commitTime: (string) (len=29) "Wed, 14 Jun 2017 13:54:21 UTC"
                                  }
                Test:           TestViewRepo2

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

I'll probably religiously test these things before sending in anything else, no matter how small, for review, ever.

Just a small heads-up, changing the strings seems to be affecting the default strings used in commit messages. This means that the projects of an instance hosted on e.g. Gitea v1.19 -> Gitea v1.20 will have different commit messages, which may hypothetically affect e.g. scraping commit summaries for information (instead of using the instance's APIs). Is this a bad thing?

@n0toose
Copy link
Contributor Author

n0toose commented Apr 15, 2023

Maybe it's best to specifically retain single-quotes in commit messages that list files. Would that be fine, @silverwind?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 16, 2023
@n0toose
Copy link
Contributor Author

n0toose commented Apr 16, 2023

Wait, once again, are we sure about this? We could do this the GitHub way and have no quotes in commit messages created by the web UI that contain filenames.

@n0toose
Copy link
Contributor Author

n0toose commented Apr 16, 2023

Pushing a new commit in a sec... (EDIT: Some technical problems came up with the repo search function once I tried to remove the quotes from the files, testing further. EDIT 2: Possibly not related to this change, pushing and marking as "ready for review".)

@n0toose n0toose marked this pull request as draft April 16, 2023 15:40
Also removes quotes in commit messages related to file modifications
made in the Web UI.
@n0toose n0toose marked this pull request as ready for review April 16, 2023 16:32
@silverwind
Copy link
Member

silverwind commented Apr 17, 2023

Maybe it's best to specifically retain single-quotes in commit messages that list files. Would that be fine, @silverwind?

Can you show an example what you mean?

Quote removal in commit messages like Add <file> is fine with me, it also matches GitHub's message when editing on web.

@n0toose
Copy link
Contributor Author

n0toose commented Apr 17, 2023

Quote removal in commit messages like Add is fine with me, it also matches GitHub's message when editing on web.

My latest commits actually follow this principle, should be good to go now. I did it this way because quotes seem to be causing a lot of trouble from locale to locale.

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 17, 2023
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Apr 17, 2023
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 17, 2023
@yardenshoham yardenshoham added this to the 1.20.0 milestone Apr 17, 2023
@techknowlogick techknowlogick enabled auto-merge (squash) April 17, 2023 21:09
@techknowlogick
Copy link
Member

🤖 🎺

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 17, 2023

🚀

@techknowlogick techknowlogick merged commit 2ef6ac8 into go-gitea:main Apr 17, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 17, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 18, 2023
* giteaofficial/main: (25 commits)
  zh-cn support on doc pages (go-gitea#24166)
  [skip ci] Updated translations via Crowdin
  Use double quotes consistently in en-US (go-gitea#24141)
  Use correct locale key for forks page (go-gitea#24172)
  Improve Wiki TOC (go-gitea#24137)
  Localize activity heatmap (except tooltip) (go-gitea#24131)
  Support triggering workflows by wiki related events (go-gitea#24119)
  add CLI command to register runner tokens (go-gitea#23762)
  Add new user types `reserved`, `bot`, and `remote` (go-gitea#24026)
  Fix Org edit page bugs: renaming detection, maxlength (go-gitea#24161)
  Make HAS_GO a simply expanded variable (go-gitea#24169)
  Support converting varchar to nvarchar for mssql database (go-gitea#24105)
  Fix math and mermaid rendering bugs (go-gitea#24049)
  Refactor locale number (go-gitea#24134)
  [skip ci] Updated translations via Crowdin
  Use 1.18's aria role for dropdown menus (go-gitea#24144)
  Set EasyMDE heading font-size to the same size as the resulting markdown (go-gitea#24151)
  Fix 2-dot direct compare to use the right base commit (go-gitea#24133)
  Add migration to fix external unit access mode of owner/admin team (go-gitea#24117)
  Remove untranslatable `on_date` key (go-gitea#24106)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/translation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants