-
Notifications
You must be signed in to change notification settings - Fork 50
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
UDENG-133 Fixing some i18n problems #658
Conversation
We use the i18n package both in client.New and in daemon.New. This happens before we initialize the domain with the right locale, so it results in improper translations. Co-authored-by: Edu Gómez Escandell <[email protected]>
Using `` is not recognized by the i18n package, so we were printing nothing in the message. Co-authored-by: Edu Gómez Escandell <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #658 +/- ##
==========================================
- Coverage 84.96% 84.94% -0.03%
==========================================
Files 74 75 +1
Lines 8043 8056 +13
==========================================
+ Hits 6834 6843 +9
- Misses 910 912 +2
- Partials 299 301 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The message is already translated a few lines earlier.
This fix was auto-generated with https://github.com/EduardGomezEscandell/go-gettext-adapter
Sudoers path: %s | ||
PolicyKit path: %s | ||
Apparmor path: %s`), updateMachine, updateUsers, nextRefresh, | ||
status := fmt.Sprintf(i18n.G("%s\n%s\nNext Refresh: %s\n\n%s\n\nActive Directory:\n %s\n\nDaemon:\n Timeout after %s\n Listening on: %s\n Cache path: %s\n Run path: %s\n Dconf path: %s\n Sudoers path: %s\n PolicyKit path: %s\n Apparmor path: %s"), updateMachine, updateUsers, nextRefresh, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sad that we have to lose in readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, can we consider/research any other approach to extract the sentences in po files and to be compatible with bacquote?
(I already personnally dislike the too many unamed "%s" in the middle of sentences, this makes it even harder to maintain in the long term)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One requested change, and I think we should, before merging this, consider if we can’t search for an alternative on string extractions.
@@ -22,6 +22,7 @@ import ( | |||
|
|||
func main() { | |||
var a app | |||
i18n.InitI18nDomain(consts.TEXTDOMAIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the a declaration is for the daemon below. So I would just move that one up. It’s not C89 :)
Sudoers path: %s | ||
PolicyKit path: %s | ||
Apparmor path: %s`), updateMachine, updateUsers, nextRefresh, | ||
status := fmt.Sprintf(i18n.G("%s\n%s\nNext Refresh: %s\n\n%s\n\nActive Directory:\n %s\n\nDaemon:\n Timeout after %s\n Listening on: %s\n Cache path: %s\n Run path: %s\n Dconf path: %s\n Sudoers path: %s\n PolicyKit path: %s\n Apparmor path: %s"), updateMachine, updateUsers, nextRefresh, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, can we consider/research any other approach to extract the sentences in po files and to be compatible with bacquote?
(I already personnally dislike the too many unamed "%s" in the middle of sentences, this makes it even harder to maintain in the long term)
61cb163
to
3776607
Compare
And with the new i18n, I think we can drop this one! |
We had some issues related to the i18n package. This PR aims to solve them.