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

Improve multiple strings in en-US locale #26213

Merged
merged 44 commits into from
Aug 8, 2023

Conversation

n0toose
Copy link
Contributor

@n0toose n0toose commented Jul 28, 2023

I kept sending pull requests that consisted of one-line changes. It's time to
settle this once and for all. (Maybe.)

  • Explain Gitea behavior and the consequences of each
    setting better, so that the user does not have to consult
    the docs.
  • Do not use different spellings of identical terms
    interchangeably, e.g. e-mail and email.
  • Use more conventional terms to describe the same things,
    e.g. Confirm Password instead of Re-Type Password.
  • Introduces additional clarification for Mirror Settings
  • Small adjustments in test
  • This is a cry for help.
  • Grammar and spelling consistencies for en-US locale
    (e.g. cancelled -> canceled)
  • Introduce tooltip improvements.

I kept sending pull requests that consisted of one-line changes. It's time to
settle this once and for all. (Maybe.)
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 28, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 28, 2023
@n0toose
Copy link
Contributor Author

n0toose commented Jul 28, 2023

I understand that all of these changes at once make for a stressful review process, but I figured that incremental fixes (as I have done so far) probably take up more resources on both ends here. I tried to go for being consistent; I think my biggest hesitation here involved the way I appended the word "successfully".

I'm a details person; I hope that this will make Gitea look more polished. I will not be available in the next few days (as in, it might take at least 12 hours for me to see replies), so feel free to merge any suggestions without my permission.

n0toose and others added 11 commits July 29, 2023 10:08
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@n0toose
Copy link
Contributor Author

n0toose commented Jul 29, 2023

Thanks for the additional recommendations and sorry for that regex fail, really goes on to show how this is super subjective work.

@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 Jul 29, 2023
@@ -691,7 +691,7 @@ emails = Email Addresses
manage_emails = Manage Email Addresses
manage_themes = Select default theme
manage_openid = Manage OpenID Addresses
email_desc = Your primary email address will be used for notifications and other operations.
email_desc = Your primary email address will be used for notifications and Git operations made using the web interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

The sentence reads strange, because IMO:

  1. Not only "Git operations made using the web interface", but all related "Git operations"
  2. Not only notifications, but I think there are also emails for verification or password recovery?

Copy link
Contributor Author

@n0toose n0toose Jul 29, 2023

Choose a reason for hiding this comment

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

Not only "Git operations made using the web interface", but all related "Git operations"

If I make a commit using a secondary email that is not primary, my primary email address will not be used. Unless if you mean pull requests.

Not only notifications, but I think there are also emails for verification or password recovery?

Good point.

Copy link
Contributor

@wxiaoguang wxiaoguang Jul 29, 2023

Choose a reason for hiding this comment

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

Not only "Git operations made using the web interface", but all related "Git operations"

If I make a commit using a secondary email that is not primary, my primary email address will not be used.

I see your point and I misunderstood it before. I think it means "this email will be used as the committer email for web Git operations"

Suggested change
email_desc = Your primary email address will be used for notifications and Git operations made using the web interface.
email_desc = Your primary email address will be used for notifications and as the committer email for web Git operations.

Like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to web-based, I think it's very much clear as day and I don't think it will confuse more advanced users that would presumably use the tea CLI tool later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind, there's many users (myself included, 2 months ago) that do not get that hiding the email address results in a noreply email address being used - this should be mentioned somewhere. It's why I made that change with the padlock - the recommendation to add a tooltip that someone made to the PR (as well as a previous commit that would never make emails accessible to everyone on the Internet) has vastly improved the situation, as people are less likely to hide it. Maybe a tooltip over the "Hide Email Address" option could help later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

32ce04c makes the text much longer, but I think that the consequences of email-related settings are very well defined. I hope that we won't have to revisit this later down the line.

Copy link
Contributor

Choose a reason for hiding this comment

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

The sentence will be used for .... and, provided that it is not hidden, web-based Git operations reads a little difficult to me. But I am not a native English speaker, so I can't comment on it (aka, no more option from my side).

Copy link
Contributor Author

@n0toose n0toose Jul 30, 2023

Choose a reason for hiding this comment

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

I don't think I can come up with a better combination of being accurate, easy to read and being transparent. I'm not a native speaker either, and we have to consider that many will use the software in this locale despite not being native speakers, so I'm definitely open to recommendations.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Aug 5, 2023
@n0toose n0toose requested a review from wxiaoguang August 5, 2023 21:22
@n0toose
Copy link
Contributor Author

n0toose commented Aug 5, 2023

Asked all three of you for reviews because I either want a final confirmation that I addressed some comments correctly or because there have been a LOT of changes ever since the "Approval". I apologize in advance for the duplicated effort.

I tried my best not to address the parameter problems for the moment, as that seems like an entirely separate issue that is out of my reach as a contributor.

@@ -3369,17 +3372,17 @@ settings.delete.success = The package has been deleted.
settings.delete.error = Failed to delete the package.
owner.settings.cargo.title = Cargo Registry Index
owner.settings.cargo.initialize = Initialize Index
owner.settings.cargo.initialize.description = To use the Cargo registry a special index git repository is needed. Here you can (re)create it with the required config.
owner.settings.cargo.initialize.description = A special index Git repository is needed to use the Cargo registry. Using this option will (re-)create it with all required configurations.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't configuration a word like sheep or fish whose plural is its singular?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct. Bear in mind that I work on this PR very late at night so that I can actually handle the mundane-ness of it, which is good for progress but still results in many little errors. I'll fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm,

I think we are both right, but it may be the case that using plural in the first place makes the idea that is meant to be conveyed here too inaccurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried something else here: faf68dc

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 5, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Thank you for inviting me to review, while I am not an English native speaker, I think people understand English better could help.

@n0toose n0toose requested a review from delvh August 6, 2023 21:04
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Aug 6, 2023
@silverwind
Copy link
Member

silverwind commented Aug 7, 2023

I'm 95% sure it's not an issue because changing a translation in english will invalidate all existing translations and it will fall back to english until translated again.

If those are not invalid, they maybe missed to translate? I don't know whether crowdin will detec that key have been updated and ask all transations to update the key.

But changing the key can do that otherwise it will not work well.

I think it's default behaviour of crowdin to invalidate existing translation when the primary value changes. This is this option which by default is not set, so I assume we don't set it (but I can't be sure because without a crowdin.yml, this could be a ui config or set by the jonasfranz/crowdin docker image) and I recommend to never set it.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 8, 2023
@denyskon
Copy link
Member

denyskon commented Aug 8, 2023

Can we merge this? We could infinitely continue adding stuff here if we wanted, but I think we should merge

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 8, 2023
@silverwind silverwind enabled auto-merge (squash) August 8, 2023 15:16
@silverwind silverwind merged commit daf7092 into go-gitea:main Aug 8, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 8, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 9, 2023
* upstream/main:
  [skip ci] Updated translations via Crowdin
  fix artifact merging chunks path with correct slash on Windows (go-gitea#26400)
  Use flex classes in package settings (go-gitea#26314)
  Improve multiple strings in en-US locale (go-gitea#26213)
  Refactor "editorconfig" (go-gitea#26391)
  fix generated source URL on rendered files (go-gitea#26364)
  Remove unnecessary template helper DisableGravatar (go-gitea#26386)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 6, 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants