HiveBrain v1.2.0
Get Started
← Back to all entries
patternrustMinor

Rust language tag implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
rustimplementationtaglanguage

Problem

language-tags is my first crate, it is hosted on GitHub and I uploaded it to Crates.io. It parses language tags like en-US into useful parts, like language and region for example. These tags then can be compared or printed out again. To create language tags it provides the langtag! macro.

Is it good code, where can I improve it?

Language tags always have a canonical captialization while being case-insensitive, should I always print the canonical form?

#![deny(missing_docs)]
#![cfg_attr(test, deny(warnings))]

//! Language tags can be used identify human languages, scripts e.g. Latin script, countries and
//! other regions.
//!
//! Language tags are defined in [BCP47](http://tools.ietf.org/html/bcp47), an introduction is
//! ["Language tags in HTML and XML"](http://www.w3.org/International/articles/language-tags/) by
//! the W3C. They are commonly used in HTML and HTTP `Content-Language` and `Accept-Language`
//! header fields.
//!
//! This package currently supports parsing (fully conformant parser), formatting and comparing
//! language tags.
//!
//! # Examples
//! Create a simple language tag representing the French language as spoken
//! in Belgium and print it:
//!
//! 
rust
//! use language_tags::LanguageTag;
//! let mut langtag: LanguageTag = Default::default();
//! langtag.language = Some("fr".to_owned());
//! langtag.region = Some("BE".to_owned());
//! assert_eq!(format!("{}", langtag), "fr-BE");
//!
//!
//! Parse a tag representing a special type of English specified by private agreement:
//!
//! 
rust
//! use language_tags::LanguageTag;
//! let langtag: LanguageTag = "en-x-twain".parse().unwrap();
//! assert_eq!(format!("{}", langtag.language.unwrap()), "en");
//! assert_eq!(format!("{:?}", langtag.privateuse), "[\"twain\"]");
//!
//!
//! You can check for equality, but more often you should test if two tags match.
//!
//! 
rust
//! use language_tags::LanguageTag;
//! let mut langtag_server: LanguageTag = Default::default();
//! la

Solution

I'm not so sure an error type should have a Display implementation. You could implement the Debug trait manually or let the user print the error through the description method.

The Error trait actually requires a Display impl.

Fields in rust are snake_case. I suggest you change privateuse to private_use.

The LanguageTag::matches method has an optically confusing return statement. I'd write it as

return matches_option(&self.language, &other.language)
    && self.extlangs.iter().all(|x|
        other.extlangs.iter().all(|y|
            x.eq_ignore_ascii_case(y)
        )
    ) && matches_option(&self.script, &other.script)
    && matches_option(&self.region, &other.region);


Same goes for the other return statements. Putting the operator at the beginning of the next line makes it easier to skim the meaning. Indentation also helps visually differentiate from a new statement.

Instead of matching on .is_some() and then right after using .as_ref().unwrap(), use destructuring. As an example I rewrote the matches_option function

fn matches_option(a: &Option, b: &Option) -> bool {
    match (a, b) {
        (&Some(ref a), &Some(ref b)) => a.eq_ignore_ascii_case(b),
        (&Some(_), &None) => false,
        (&None, _) => true,
    }
}


The following Default impl screams after #[derive(Default)]:

impl Default for LanguageTag {
    fn default() -> LanguageTag {
        LanguageTag {
            language: None,
            extlangs: Vec::new(),
            script: None,
            region: None,
            variants: Vec::new(),
            extensions: BTreeMap::new(),
            privateuse: Vec::new(),
        }
    }
}

Code Snippets

return matches_option(&self.language, &other.language)
    && self.extlangs.iter().all(|x|
        other.extlangs.iter().all(|y|
            x.eq_ignore_ascii_case(y)
        )
    ) && matches_option(&self.script, &other.script)
    && matches_option(&self.region, &other.region);
fn matches_option(a: &Option<String>, b: &Option<String>) -> bool {
    match (a, b) {
        (&Some(ref a), &Some(ref b)) => a.eq_ignore_ascii_case(b),
        (&Some(_), &None) => false,
        (&None, _) => true,
    }
}
impl Default for LanguageTag {
    fn default() -> LanguageTag {
        LanguageTag {
            language: None,
            extlangs: Vec::new(),
            script: None,
            region: None,
            variants: Vec::new(),
            extensions: BTreeMap::new(),
            privateuse: Vec::new(),
        }
    }
}

Context

StackExchange Code Review Q#95890, answer score: 4

Revisions (0)

No revisions yet.