Add config file option to set preferred language #318
Conversation
src/main.rs
Outdated
| use std::iter::once; | ||
| use std::{env, process}; |
There was a problem hiding this comment.
Nit: Can you merge these imports for consistency?
use std::{env, iter::once, process};
| env_language.chain(once(env_lang)).collect() | ||
| } else { | ||
| env_language.collect() | ||
| }; |
There was a problem hiding this comment.
I don't think this logic conforms to https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md#language anymore, because the fast-path (if env_lang.is_none() { return ... }) was removed.
| #[test] | ||
| fn cli_preference_order() { | ||
| let lang_list = get_languages(Some("it"), Some("cz"), Some("de"), Some("fr:cn")); | ||
| assert_eq!(lang_list, ["it", "en"]) | ||
| } |
There was a problem hiding this comment.
This deviates from current behavior: Running tldr --language de git-checkout does not display the English page (and there is no German page currently). However, this test here says that English pages should be used. This boils down to the changes in get_languages - when I first wrote this method, it was meant to exactly follow the spec regarding the whole $LANG and $LANGUAGE business. I will make another comment there to suggest how to structure it going forward
| /// Return language(s) with the following precedence: | ||
| /// | ||
| /// 1. CLI language parameter (`-L`, `--language`) | ||
| /// 2. Language as set in the config file | ||
| /// 3. If the `$LANG` env var is set: `$LANGUAGE` (list separated by `:`) followed by `$LANG` (single language) | ||
| /// 4. Fallback: English |
There was a problem hiding this comment.
This is not exactly what we want - the "Fallback: English" should only apply to the environment variables, not to the CLI or config file setting. Hence I suggest the following:
- Leave
get_languagesas is, maybe only rename it toget_languages_from_envor so and remove the currentget_languages_from_env - Make a new function that takes
argsandconfigand triesargs, thenconfigand finally usesget_languages_from_env. - To allow for testing without setting environment variables, the new function should only redirect to another function with a signature like your current
get_languages:
fn the_new_function_impl(
command_line_lang: Option<&str>,
config_lang: Option<&str>,
env_lang: Option<&str>,
env_language: Option<&str>,
) -> Vec<String> {
|
@cjwcommuny Do you want to continue working on this, or should we close this PR for now? |
This pull request enable users to write the following config:
to override the language.
Closes #251.