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

Report distinction between default and non-default languages #12

Merged
merged 4 commits into from
Aug 19, 2023

Conversation

peter-scholtens
Copy link
Contributor

For a webserver, where new translation languages may appear after the initial release, it will be handy not to stick users to their non-default best match. This implements the request at issue #9.

For a webserver, where new translation languages may appear after the initial release, it will be handy not to stick users to their non-default best match.
@peter-scholtens
Copy link
Contributor Author

Maybe it would be more useful for other users of the crate to return the qualification? So instead of this patch:

pub fn parse_verify_default(raw_languages: &str) -> Vec<(String, bool)>
pub fn intersection_verify_default(raw_languages: &str, supported_languages: &[&str]) -> Vec<(String, bool)> 

I could re-write it to:

pub fn parse_with_qualification(raw_languages: &str) -> Vec<(String, f32)>
pub fn intersection_with_qualification(raw_languages: &str, supported_languages: &[&str]) -> Vec<(String, f32)> 

Although that makes the optimized one (not present yet) even longer:

pub fn parse_ordered_with_qualification(raw_languages: &str) -> Vec<(String, f32)>
pub fn intersection_ordered_with_qualification(raw_languages: &str, supported_languages: &[&str]) -> Vec<(String, f32)> 

Any other suggestions?

@mike-engel
Copy link
Owner

@peter-scholtens Your potential change makes sense to me. Returning the quality is more flexible, and consumers could do more than check for default if they want. In terms of method length, I don't really care about it since I would rather prioritize readability and clarity over terseness. Rust mangles the name during compilation anyway, so this is a user-facing concern only

…d to scan a correctly binary sorted array of languages faster. Intersections functions with qualification in output added.
@peter-scholtens
Copy link
Contributor Author

@mike-engel patch-2 (2513935) contain the agreed function signatures and modification. Ready for a merge and release I would say :-)

src/lib.rs Outdated
/// let user_languages = parse_with_qualification("en-US, en-GB;q=0.5");
/// assert_eq!(user_languages,vec![(String::from("en-US"), 1.0), (String::from("en-GB"), 0.5)])
/// ```
pub fn parse_with_qualification(raw_languages: &str) -> Vec<(String, f32)> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm wondering now if parse, intersection, and intersection_ordered should just call parse_with_qualification, intersection_with_qualification, and intersection_ordered_with_qualification and map over the return type to change it from Vec<(String, f32)> to Vec<String>? The function bodies are almost exactly the same, minus the map closure.

Additionally, I think quality is the more standard term over qualification, which would also shorten the names a little bit 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. But that will also means a slower implementation for the user with a frugal request without quality. Another option would be to re-write it with macro's, so you get the fastest implementation for with and without quality, but I've no experience in writing them: possibly it will become less readable as code?

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not too familiar with macros either, so let's leave it as-is for now 😄

@mike-engel mike-engel merged commit 2b01693 into mike-engel:main Aug 19, 2023
@mike-engel
Copy link
Owner

Thanks @peter-scholtens this is now released as 3.1.0 and on crates.io!

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