Skip to content

features: Distinguish between optional dependencies and features #1180

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 17, 2020
Merged

features: Distinguish between optional dependencies and features #1180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2039231
features: Differ optional dependencies and features
almusil Nov 16, 2020
c296bc2
features: Document the new optional_dependency field
almusil Nov 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/db/add_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,13 +221,13 @@ fn convert_dependencies(pkg: &MetadataPackage) -> Vec<(String, String, String)>
fn get_features(pkg: &MetadataPackage) -> Vec<Feature> {
let mut features = Vec::with_capacity(pkg.features.len());
if let Some(subfeatures) = pkg.features.get("default") {
features.push(Feature::new("default".into(), subfeatures.clone()));
features.push(Feature::new("default".into(), subfeatures.clone(), false));
};
features.extend(
pkg.features
.iter()
.filter(|(name, _)| *name != "default")
.map(|(name, subfeatures)| Feature::new(name.clone(), subfeatures.clone())),
.map(|(name, subfeatures)| Feature::new(name.clone(), subfeatures.clone(), false)),
);
features.extend(get_optional_dependencies(pkg));
features
Expand All @@ -237,7 +237,14 @@ fn get_optional_dependencies(pkg: &MetadataPackage) -> Vec<Feature> {
pkg.dependencies
.iter()
.filter(|dep| dep.optional)
.map(|dep| Feature::new(dep.name.clone(), Vec::new()))
.map(|dep| {
let name = if let Some(rename) = &dep.rename {
rename.clone()
} else {
dep.name.clone()
};
Feature::new(name, Vec::new(), true)
})
.collect()
}

Expand Down
14 changes: 14 additions & 0 deletions src/db/migrate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,20 @@ pub fn migrate(version: Option<Version>, conn: &mut Client) -> CratesfyiResult<(
"
ALTER TABLE queue DROP COLUMN registry;
"
),
migration!(
context,
21,
// description
"Add mark for features that are derived from optional dependencies",
// upgrade query
"
ALTER TYPE feature ADD ATTRIBUTE optional_dependency BOOL;
Copy link
Member

Choose a reason for hiding this comment

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

Where can I learn more about postgres data types? Will optional_dependency be added as NULL? It should be.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, which is an issue when loading the features page for existing crates:

thread '<unnamed>' panicked at 'error retrieving column 0: error deserializing column 0: a Postgres value was `NULL`'

Probably the type needs to be changed to optional_dependency: Option<bool>.

Copy link
Contributor Author

@almusil almusil Nov 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I have realized that later. IMO it would make sense to make is false as default.

EDIT: Nevermind it seems like we cannot set default in ALTER TYPE. At least I was not able to find it in documentation.

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 shouldn't say something we don't know for sure. We should show an optional dependency/feature tag if the info is there, but just show nothing if we don't know.

",
// downgrade query
"
ALTER TYPE feature DROP ATTRIBUTE optional_dependency;
"
)
];

Expand Down
10 changes: 8 additions & 2 deletions src/db/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,17 @@ use serde::Serialize;
pub struct Feature {
pub(crate) name: String,
pub(crate) subfeatures: Vec<String>,
/// [`None`] when this crate was built before optional dependencies were tracked
pub(crate) optional_dependency: Option<bool>,
}

impl Feature {
pub fn new(name: String, subfeatures: Vec<String>) -> Self {
Feature { name, subfeatures }
pub fn new(name: String, subfeatures: Vec<String>, optional_dependency: bool) -> Self {
Feature {
name,
subfeatures,
optional_dependency: Some(optional_dependency),
}
}

pub fn is_private(&self) -> bool {
Expand Down
1 change: 1 addition & 0 deletions src/test/fakes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ impl<'a> FakeRelease<'a> {
name: "fake-dependency".into(),
req: "^1.0.0".into(),
kind: None,
rename: None,
optional: false,
}],
targets: vec![Target::dummy_lib("fake_package".into(), None)],
Expand Down
1 change: 1 addition & 0 deletions src/utils/cargo_metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ pub(crate) struct Dependency {
pub(crate) name: String,
pub(crate) req: String,
pub(crate) kind: Option<String>,
pub(crate) rename: Option<String>,
pub(crate) optional: bool,
}

Expand Down
31 changes: 17 additions & 14 deletions src/web/features.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ mod tests {

#[test]
fn test_feature_map_filters_private() {
let private1 = Feature::new("_private1".into(), vec!["feature1".into()]);
let feature2 = Feature::new("feature2".into(), Vec::new());
let private1 = Feature::new("_private1".into(), vec!["feature1".into()], false);
let feature2 = Feature::new("feature2".into(), Vec::new(), false);

let raw = vec![private1.clone(), feature2.clone()];
let feature_map = get_feature_map(raw);
Expand All @@ -120,14 +120,15 @@ mod tests {

#[test]
fn test_default_tree_structure_with_nested_default() {
let default = Feature::new(DEFAULT_NAME.into(), vec!["feature1".into()]);
let non_default = Feature::new("non-default".into(), Vec::new());
let default = Feature::new(DEFAULT_NAME.into(), vec!["feature1".into()], false);
let non_default = Feature::new("non-default".into(), Vec::new(), false);
let feature1 = Feature::new(
"feature1".into(),
vec!["feature2".into(), "feature3".into()],
false,
);
let feature2 = Feature::new("feature2".into(), Vec::new());
let feature3 = Feature::new("feature3".into(), Vec::new());
let feature2 = Feature::new("feature2".into(), Vec::new(), false);
let feature3 = Feature::new("feature3".into(), Vec::new(), false);

let raw = vec![
default.clone(),
Expand All @@ -154,9 +155,10 @@ mod tests {
let feature1 = Feature::new(
"feature1".into(),
vec!["feature2".into(), "feature3".into()],
false,
);
let feature2 = Feature::new("feature2".into(), Vec::new());
let feature3 = Feature::new("feature3".into(), Vec::new());
let feature2 = Feature::new("feature2".into(), Vec::new(), false);
let feature3 = Feature::new("feature3".into(), Vec::new(), false);

let raw = vec![feature3.clone(), feature2.clone(), feature1.clone()];
let mut feature_map = get_feature_map(raw);
Expand All @@ -171,8 +173,8 @@ mod tests {

#[test]
fn test_default_tree_structure_single_default() {
let default = Feature::new(DEFAULT_NAME.into(), Vec::new());
let non_default = Feature::new("non-default".into(), Vec::new());
let default = Feature::new(DEFAULT_NAME.into(), Vec::new(), false);
let non_default = Feature::new("non-default".into(), Vec::new(), false);

let raw = vec![default.clone(), non_default.clone()];
let mut feature_map = get_feature_map(raw);
Expand All @@ -190,9 +192,10 @@ mod tests {
let feature1 = Feature::new(
"feature1".into(),
vec!["feature10".into(), "feature11".into()],
false,
);
let feature2 = Feature::new("feature2".into(), vec!["feature20".into()]);
let feature3 = Feature::new("feature3".into(), Vec::new());
let feature2 = Feature::new("feature2".into(), vec!["feature20".into()], false);
let feature3 = Feature::new("feature3".into(), Vec::new(), false);

let raw = vec![feature3.clone(), feature2.clone(), feature1.clone()];
let (features, default_len) = order_features_and_count_default_len(raw);
Expand All @@ -206,8 +209,8 @@ mod tests {

#[test]
fn test_order_features_and_get_len_single_default() {
let default = Feature::new(DEFAULT_NAME.into(), Vec::new());
let non_default = Feature::new("non-default".into(), Vec::new());
let default = Feature::new(DEFAULT_NAME.into(), Vec::new(), false);
let non_default = Feature::new("non-default".into(), Vec::new(), false);

let raw = vec![default.clone(), non_default.clone()];
let (features, default_len) = order_features_and_count_default_len(raw);
Expand Down