Conversation
|
On it |
Summary
Testing
|
|
On it |
|
Summary
Testing
|
samcday
added
ci-ok
|
@codex so this is embarrassing but nothing is really building yet since the crate name appears to be made up. It's actually gettext-rs, I think? And so anyway I enabled your internet access so I promise you can try harder to produce a valid build. maybe don't end your turn until |
|
Summary
Testing
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@codex flatten this back down to some sensible commits:
|
This comment has been minimized.
This comment has been minimized.
|
Codex couldn't complete this request. Try again later. |
samcday
force-pushed
the
codex/github-mention-add-phosh-gettext-domain
branch
from
aa57360 to
3cf90f8
Compare
This comment has been minimized.
This comment has been minimized.
samcday
changed the title
tests/common/mod.rs
Outdated
| let locale_dir = options | ||
| .as_ref() | ||
| .and_then(|opts| opts.locale_dir.as_deref()) | ||
| .unwrap_or_else(|| std::path::Path::new(phrog::LOCALEDIR)); | ||
| gettextrs::textdomain("phosh").unwrap(); | ||
| gettextrs::bind_textdomain_codeset("phosh", "UTF-8").unwrap(); | ||
| gettextrs::bindtextdomain("phosh", locale_dir).unwrap(); | ||
| gettextrs::bind_textdomain_codeset(phrog::TEXT_DOMAIN, "UTF-8").unwrap(); | ||
| gettextrs::bindtextdomain(phrog::TEXT_DOMAIN, locale_dir).unwrap(); |
There was a problem hiding this comment.
As this got shuffled out in all the excitement:
Let's move this into a fn i18n_setup -> Result, and ensure that we do not fail to startup the app if something croaked in gettext-pokery.
tests/swedish_chef.rs
Outdated
| gettextrs::bindtextdomain(TEXT_DOMAIN, phrog::LOCALEDIR).unwrap(); | ||
| restore_env_var("LANGUAGE", old_language); | ||
| restore_env_var("LANG", old_lang); | ||
| restore_env_var("LC_ALL", old_lc_all); | ||
| restore_env_var("LC_MESSAGES", old_lc_messages); | ||
| gettextrs::setlocale(gettextrs::LocaleCategory::LcAll, "") | ||
| .expect("failed to restore locale after gettext tests"); | ||
| } |
There was a problem hiding this comment.
I just know this sort of brittle shit is gonna haunt some poor bastard (probably me) later.
Can we do something like, idk. less shit here? I'm guessing that we're already gonna have problems with running all our tests in a single process instance with gettext.
... Then again. Does gettext-rs have a sane teardown path?
If so - what if we bind the locale setup to the main phrog::Shell instantiation (and then clean it up on teardown)? That would also make it trivial for us to pass in a custom locale as constructor-only props (rather than monkeying with the env vars)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cf90f8d1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tests/swedish_chef.rs
Outdated
| std::env::set_var("LANG", "en_US.UTF-8"); | ||
| std::env::remove_var("LC_ALL"); | ||
| std::env::remove_var("LC_MESSAGES"); | ||
| gettextrs::setlocale(gettextrs::LocaleCategory::LcAll, "") | ||
| .expect("failed to activate en_US.UTF-8 locale (gettext ignores LANGUAGE in C locale)"); |
There was a problem hiding this comment.
Setting LANG to en_US.UTF-8 and then unconditionally expecting setlocale(..., "") to succeed makes this test fail on environments where that locale is not generated (common in minimal CI/container images that only provide C/C.UTF-8). In that case setlocale returns None, the expect(...) panics, and the i18n test fails before exercising the feature under test.
Useful? React with 👍 / 👎.
|
@codex no silly I wanted you to cook up another commit that addresses the feedback I provided (and I guess, the stuff you provided too? though not sure what's to be done there, we want tests to fail if the environment lacks something necessary for them to succeed). |
|
Summary
Testing
|
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
samcday
force-pushed
the
codex/github-mention-add-phosh-gettext-domain
branch
from
920830a to
e46b036
Compare
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
Signed-off-by: Sam Day <me@samcday.com> Assisted-by: opencode:gpt-5.3-codex
|
The CI build recorded and generated some videos Demo video (shown on README and release notes)
accent-colours
emergency-calls
first-run
simple-flow
swedish-chef
trivial-flow
|
1 participant
Motivation
phoshtranslations remain the default while adding a separate gettext domain for phrog-specific strings.phrogdomain per reviewer guidance to avoid mixing domains.Description
gettextrs = "0.7"toCargo.tomlto support gettext operations from Rust.GETTEXT_PACKAGE("phrog") andLOCALEDIR("/usr/share/locale") constants insrc/lib.rsfor shared i18n configuration.src/main.rsinitialize locale viagettextrs::setlocale, bind bothphoshandphrogdomains withgettextrs::bindtextdomain, and keepphoshas the activetextdomain, logging warnings on failure rather than failing startup.gettextrs::dgettext("phrog", "Password:")insrc/lockscreen.rs.Testing
cargo fmtsuccessfully to format the changes.cargo checkbut it failed in this environment due to a network/crates.io index error (CONNECT tunnel failed, response 403).Codex Task