Skip to content

Commit

Permalink
Allow feature names to begin with numbers
Browse files Browse the repository at this point in the history
Diesel 1.2 had planned on renaming our `large-tables` and `huge-tables`
features to `32-column-tables` and `64-column-tables` respectively,
while also introducing the `128-column-tables` feature. This change was
made several months ago in Diesel. Cargo will happily accept those
as feature names, and resolve them properly from other crates.

However while publishing Diesel 1.2, I ran into a snag mid-release when
I realized that Cargo is incorrectly assuming that a feature name must
be the same as a crate name. I suspect this is an artifact of the fact
that feature names often are crate names (and perhaps in the past that
was the only form of feature?).

However, now they are an entirely separate thing, and we should allow
the same set of feature names that Cargo does. (As an aside, do we want
to apply the same 64 character limit that we apply to crate names to
feature names?)

Fixes #1329.
  • Loading branch information
sgrif committed Apr 7, 2018
1 parent a093b70 commit 4bf9efa
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 13 deletions.
19 changes: 12 additions & 7 deletions src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,24 +250,29 @@ impl Crate {
}

fn valid_ident(name: &str) -> bool {
if name.is_empty() {
return false;
}
name.chars().next().unwrap().is_alphabetic()
Self::valid_feature_name(name)
&& name.chars()
.nth(0)
.map(char::is_alphabetic)
.unwrap_or(false)
}

pub fn valid_feature_name(name: &str) -> bool {
!name.is_empty()
&& name.chars()
.all(|c| c.is_alphanumeric() || c == '_' || c == '-')
&& name.chars().all(|c| c.is_ascii())
}

pub fn valid_feature_name(name: &str) -> bool {
pub fn valid_feature(name: &str) -> bool {
let mut parts = name.split('/');
match parts.next() {
Some(part) if !Crate::valid_ident(part) => return false,
Some(part) if !Crate::valid_feature_name(part) => return false,
None => return false,
_ => {}
}
match parts.next() {
Some(part) if !Crate::valid_ident(part) => return false,
Some(part) if !Crate::valid_feature_name(part) => return false,
_ => {}
}
parts.next().is_none()
Expand Down
11 changes: 6 additions & 5 deletions src/tests/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -753,11 +753,12 @@ fn new_krate_bad_name() {

#[test]
fn valid_feature_names() {
assert!(Crate::valid_feature_name("foo"));
assert!(!Crate::valid_feature_name(""));
assert!(!Crate::valid_feature_name("/"));
assert!(!Crate::valid_feature_name("%/%"));
assert!(Crate::valid_feature_name("a/a"));
assert!(Crate::valid_feature("foo"));
assert!(!Crate::valid_feature(""));
assert!(!Crate::valid_feature("/"));
assert!(!Crate::valid_feature("%/%"));
assert!(Crate::valid_feature("a/a"));
assert!(Crate::valid_feature("32-column-tables"));
}

#[test]
Expand Down
18 changes: 17 additions & 1 deletion src/views/krate_publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pub struct NewCrate {
pub name: CrateName,
pub vers: CrateVersion,
pub deps: Vec<CrateDependency>,
pub features: HashMap<CrateName, Vec<Feature>>,
pub features: HashMap<FeatureName, Vec<Feature>>,
pub authors: Vec<String>,
pub description: Option<String>,
pub homepage: Option<String>,
Expand Down Expand Up @@ -51,6 +51,8 @@ pub struct CategoryList(pub Vec<Category>);
pub struct Category(pub String);
#[derive(Serialize, Debug, Deref)]
pub struct Feature(pub String);
#[derive(PartialEq, Eq, Hash, Serialize, Debug, Deref)]
pub struct FeatureName(pub String);

#[derive(Serialize, Deserialize, Debug)]
pub struct CrateDependency {
Expand Down Expand Up @@ -102,6 +104,20 @@ impl<'de> Deserialize<'de> for Keyword {
}
}

impl<'de> Deserialize<'de> for FeatureName {
fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
let s = String::deserialize(d)?;
if !Crate::valid_feature_name(&s) {
let value = de::Unexpected::Str(&s);
let expected = "a valid feature name containing only letters, \
numbers, hyphens, or underscores";
Err(de::Error::invalid_value(value, &expected))
} else {
Ok(FeatureName(s))
}
}
}

impl<'de> Deserialize<'de> for Feature {
fn deserialize<D: Deserializer<'de>>(d: D) -> Result<Feature, D::Error> {
let s = String::deserialize(d)?;
Expand Down

0 comments on commit 4bf9efa

Please sign in to comment.