Conversation
|
for Ubuntu / Unix like systems, issue #55 is solved |
|
@TrifanBogdan24 thanks for opening this PR! Can you remove the Each new file should also contain some documentation on where it was sourced from, or removed if not used. Before I review further, please run cargo fmtand fix warnings/errors returned by cargo clippy |
|
Sure. I will push the corrected code ASAP. |
|
@AldaronLau, I solved the warning generated by Please double check the language and country encodings to be correct. |
|
@TrifanBogdan24 apologies it's taken me so long to get to this. When running cargo +nightly fmtIt may take me a while to get through verifying the country encodings. |
| #[allow(dead_code)] | ||
| fn get_language_and_country() -> String { | ||
| if let Ok(language) = env::var("LC_ALL") { | ||
| // possible output : en_US.UTF-8 | ||
| return language; | ||
| } else if let Ok(language) = env::var("LANG") { | ||
| // possible output : en_US.UTF-8 | ||
| return language; | ||
| } else if let Ok(language) = env::var("LANGUAGE") { | ||
| // possible output : en_US.UTF-8 | ||
| return language; | ||
| } | ||
| String::new() | ||
| } |
There was a problem hiding this comment.
For this change, could you split it out into it's own PR?
This PR is already large, and it's easier to review in smaller chunks.
AldaronLau
left a comment
There was a problem hiding this comment.
Thanks again for doing this work. I think splitting language / region into separate PRs will make it easier for me to review and get this PR merged.
|
|
||
| /// `AF`: Afghanistan | ||
| #[doc(hidden)] | ||
| Af, |
There was a problem hiding this comment.
For the two-letter country codes, #[doc(hidden)] can be removed.
|
|
||
| /// an u32 code for region | ||
| #[doc(hidden)] | ||
| Custom(u32), |
There was a problem hiding this comment.
I think we'll probably want to remove variant this before merging, and move the numeric parsing in a FromStr implementation.
| return Language::Ji(region); | ||
| } else if var_env.contains("yo") { | ||
| return Language::Yo(region); | ||
| } else if var_env.contains("zu") { |
There was a problem hiding this comment.
As for these methods, I think this needs to be rethought a little.
Once the LANG SEPARATOR COUNTRY is returned by the OS, the string should be split, so that there are two variables; lang and country. Rather than calling .contains(), they should be able to be matched exactly:
match lang {
lang if ["AF", "AFG", "004"].contains(&lang) => Language::Af(region),
_ => /* ... */,
}|
Found the ISO documentation not behind a paywall or limiting copyright at government websites: For reference, languages should be checked against https://www.loc.gov/standards/iso639-2/ISO-639-2_utf-8.txt And countries should be checked against https://www.cia.gov/the-world-factbook/references/country-data-codes/ (CSV file) I can do the checking / testing against what you have. I think the .txt files you have in this PR can be deleted. |
|
@TrifanBogdan24 are you still interested in working on this after my most recent feedback to split into multiple PRs? (Also, apologies about the merge conflicts). It would be nice to have your fix for #55 in whoami 1.5.0. Otherwise, I can split it out of this PR if you're no longer interested in working on this. Thank you |
working on : Enumerate Languages And Dialect Regions #79
for countries: https://www.iban.com/country-codes