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

Adding regions and languages #81

Open
wants to merge 9 commits into
base: v1
Choose a base branch
from

Conversation

TrifanBogdan24
Copy link

working on : Enumerate Languages And Dialect Regions #79
for countries: https://www.iban.com/country-codes

@TrifanBogdan24
Copy link
Author

TrifanBogdan24 commented Dec 28, 2023

Solved issues : #60 , #79
Problem with the first three tests though

@TrifanBogdan24 TrifanBogdan24 marked this pull request as ready for review December 28, 2023 00:08
@TrifanBogdan24
Copy link
Author

for Ubuntu / Unix like systems, issue #55 is solved

@AldaronLau
Copy link
Member

@TrifanBogdan24 thanks for opening this PR!

Can you remove the .idea folder from this PR? You can add it to the .gitignore file.

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 fmt

and fix warnings/errors returned by

cargo clippy

@TrifanBogdan24
Copy link
Author

Sure. I will push the corrected code ASAP.

@TrifanBogdan24
Copy link
Author

@AldaronLau, I solved the warning generated by cargo clippy (most of them were related to cyclomatic complexity, therefore I had to split the code in multiple functions).
I don't know how to solve cargo fmt

Please double check the language and country encodings to be correct.
Screenshot from 2024-01-09 09-33-07
Screenshot from 2024-01-09 09-32-51
Screenshot from 2024-01-09 09-32-02

@AldaronLau
Copy link
Member

@TrifanBogdan24 apologies it's taken me so long to get to this.

When running cargo fmt, use +nightly:

cargo +nightly fmt

It may take me a while to get through verifying the country encodings.

Comment on lines +123 to +136
#[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()
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@AldaronLau AldaronLau left a comment

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

For the two-letter country codes, #[doc(hidden)] can be removed.


/// an u32 code for region
#[doc(hidden)]
Custom(u32),
Copy link
Member

Choose a reason for hiding this comment

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

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") {
Copy link
Member

Choose a reason for hiding this comment

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

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),
    _ => /* ... */,
}

@AldaronLau
Copy link
Member

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.

@AldaronLau
Copy link
Member

@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

@AldaronLau AldaronLau mentioned this pull request Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants