patternrustMinor
Rust language tag implementation
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
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?
//! 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");
//!
//! 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\"]");
//!
//! use language_tags::LanguageTag;
//! let mut langtag_server: LanguageTag = Default::default();
//! la
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
The
Fields in rust are snake_case. I suggest you change
The
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
The following
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 functionfn 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.