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

Support colocating subfolders #1582

Merged
merged 2 commits into from
Sep 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions components/library/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ serde_derive = "1"
regex = "1"
lazy_static = "1"
lexical-sort = "0.3"
walkdir = "2"

front_matter = { path = "../front_matter" }
config = { path = "../config" }
Expand Down
47 changes: 23 additions & 24 deletions components/library/src/content/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ mod page;
mod section;
mod ser;

use std::fs::read_dir;
use std::path::{Path, PathBuf};

use walkdir::WalkDir;

pub use self::file_info::FileInfo;
pub use self::page::Page;
pub use self::section::Section;
Expand All @@ -32,7 +33,7 @@ pub fn has_anchor(headings: &[Heading], anchor: &str) -> bool {
pub fn find_related_assets(path: &Path, config: &Config) -> Vec<PathBuf> {
let mut assets = vec![];

for entry in read_dir(path).unwrap().filter_map(std::result::Result::ok) {
for entry in WalkDir::new(path).into_iter().filter_map(std::result::Result::ok) {
let entry_path = entry.path();
if entry_path.is_file() {
match entry_path.extension() {
Expand All @@ -46,18 +47,11 @@ pub fn find_related_assets(path: &Path, config: &Config) -> Vec<PathBuf> {
}

if let Some(ref globset) = config.ignored_content_globset {
// `find_related_assets` only scans the immediate directory (it is not recursive) so our
// filtering only needs to work against the file_name component, not the full suffix. If
// `find_related_assets` was changed to also return files in subdirectories, we could
// use `PathBuf.strip_prefix` to remove the parent directory and then glob-filter
// against the remaining path. Note that the current behaviour effectively means that
// the `ignored_content` setting in the config file is limited to single-file glob
// patterns (no "**" patterns).
Comment on lines -49 to -55
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes are centered around this advice. Above, I replaced read_dir by WalkDir to collect assets recursively. And everywhere where the collected assets are accessed I replaced .file_name() with .strip_prefix(&page.file.path.parent().unwrap()). Note that I wanted to use .strip_prefix(&page.file.parent) originally but there is a special case for pages, where parent is actually grand_parent, that breaks this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

there is a special case for pages, where parent is actually grand_parent, that breaks this.

Do you mean when you have an asset folder? That's expected then since otherwise the parent would refer to the page itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what I meant.

assets = assets
.into_iter()
.filter(|path| match path.file_name() {
None => false,
Some(file) => !globset.is_match(file),
.filter(|p| match p.strip_prefix(path) {
Err(_) => false,
Ok(file) => !globset.is_match(file),
})
.collect();
}
Expand All @@ -68,25 +62,30 @@ pub fn find_related_assets(path: &Path, config: &Config) -> Vec<PathBuf> {
#[cfg(test)]
mod tests {
use super::*;
use std::fs::File;
use std::fs::{File, create_dir};

use config::Config;
use tempfile::tempdir;

#[test]
fn can_find_related_assets() {
let tmp_dir = tempdir().expect("create temp dir");
File::create(tmp_dir.path().join("index.md")).unwrap();
File::create(tmp_dir.path().join("example.js")).unwrap();
File::create(tmp_dir.path().join("graph.jpg")).unwrap();
File::create(tmp_dir.path().join("fail.png")).unwrap();

let assets = find_related_assets(tmp_dir.path(), &Config::default());
assert_eq!(assets.len(), 3);
assert_eq!(assets.iter().filter(|p| p.extension().unwrap() != "md").count(), 3);
assert_eq!(assets.iter().filter(|p| p.file_name().unwrap() == "example.js").count(), 1);
assert_eq!(assets.iter().filter(|p| p.file_name().unwrap() == "graph.jpg").count(), 1);
assert_eq!(assets.iter().filter(|p| p.file_name().unwrap() == "fail.png").count(), 1);
let path = tmp_dir.path();
File::create(path.join("index.md")).unwrap();
File::create(path.join("example.js")).unwrap();
File::create(path.join("graph.jpg")).unwrap();
File::create(path.join("fail.png")).unwrap();
create_dir(path.join("subdir")).expect("create subdir temp dir");
File::create(path.join("subdir").join("index.md")).unwrap();
File::create(path.join("subdir").join("example.js")).unwrap();

let assets = find_related_assets(path, &Config::default());
assert_eq!(assets.len(), 4);
assert_eq!(assets.iter().filter(|p| p.extension().unwrap() != "md").count(), 4);
assert_eq!(assets.iter().filter(|p| p.strip_prefix(path).unwrap() == Path::new("example.js")).count(), 1);
assert_eq!(assets.iter().filter(|p| p.strip_prefix(path).unwrap() == Path::new("graph.jpg")).count(), 1);
assert_eq!(assets.iter().filter(|p| p.strip_prefix(path).unwrap() == Path::new("fail.png")).count(), 1);
assert_eq!(assets.iter().filter(|p| p.strip_prefix(path).unwrap() == Path::new("subdir/example.js")).count(), 1);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion components/library/src/content/page.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ impl Page {
fn serialize_assets(&self, base_path: &Path) -> Vec<String> {
self.assets
.iter()
.filter_map(|asset| asset.file_name())
.filter_map(|asset| asset.strip_prefix(&self.file.path.parent().unwrap()).ok())
.filter_map(|filename| filename.to_str())
.map(|filename| {
let mut path = self.file.path.clone();
Expand Down
2 changes: 1 addition & 1 deletion components/library/src/content/section.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ impl Section {
fn serialize_assets(&self) -> Vec<String> {
self.assets
.iter()
.filter_map(|asset| asset.file_name())
.filter_map(|asset| asset.strip_prefix(&self.file.path.parent().unwrap()).ok())
.filter_map(|filename| filename.to_str())
.map(|filename| format!("{}{}", self.path, filename))
.collect()
Expand Down
17 changes: 0 additions & 17 deletions components/rendering/src/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,6 @@ fn starts_with_schema(s: &str) -> bool {
PATTERN.is_match(s)
}

/// Colocated asset links refers to the files in the same directory,
/// there it should be a filename only
fn is_colocated_asset_link(link: &str) -> bool {
!link.contains('/') // http://, ftp://, ../ etc
&& !starts_with_schema(link)
}

Comment on lines -77 to -83
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could adjust this to match files in colocated subfolders as well. But I'm proposing to get rid of this completely instead. Why change relative links to absolute links?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I cannot remember

/// Returns whether a link starts with an HTTP(s) scheme.
fn is_external_link(link: &str) -> bool {
link.starts_with("http:") || link.starts_with("https:")
Expand Down Expand Up @@ -111,8 +104,6 @@ fn fix_link(
return Err(format!("Relative link {} not found.", link).into());
}
}
} else if is_colocated_asset_link(link) {
format!("{}{}", context.current_page_permalink, link)
} else {
if is_external_link(link) {
external_links.push(link.to_owned());
Expand Down Expand Up @@ -222,14 +213,6 @@ pub fn markdown_to_html(content: &str, context: &RenderContext) -> Result<Render
code_block = None;
Event::Html("</code></pre>\n".into())
}
Event::Start(Tag::Image(link_type, src, title)) => {
if is_colocated_asset_link(&src) {
let link = format!("{}{}", context.current_page_permalink, &*src);
return Event::Start(Tag::Image(link_type, link.into(), title));
}

Event::Start(Tag::Image(link_type, src, title))
}
Event::Start(Tag::Link(link_type, link, title)) if link.is_empty() => {
error = Some(Error::msg("There is a link that is missing a URL"));
Event::Start(Tag::Link(link_type, "#".into(), title))
Expand Down
4 changes: 2 additions & 2 deletions components/rendering/tests/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -935,7 +935,7 @@ fn can_make_permalinks_with_colocated_assets_for_link() {
InsertAnchor::None,
);
let res = render_content("[an image](image.jpg)", &context).unwrap();
assert_eq!(res.body, "<p><a href=\"https://vincent.is/about/image.jpg\">an image</a></p>\n");
assert_eq!(res.body, "<p><a href=\"image.jpg\">an image</a></p>\n");
}

#[test]
Expand All @@ -953,7 +953,7 @@ fn can_make_permalinks_with_colocated_assets_for_image() {
let res = render_content("![alt text](image.jpg)", &context).unwrap();
assert_eq!(
res.body,
"<p><img src=\"https://vincent.is/about/image.jpg\" alt=\"alt text\" /></p>\n"
"<p><img src=\"image.jpg\" alt=\"alt text\" /></p>\n"
);
}

Expand Down
4 changes: 2 additions & 2 deletions components/site/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ impl Site {
self.copy_asset(
asset_path,
&current_path
.join(asset_path.file_name().expect("Couldn't get filename from page asset")),
.join(asset_path.strip_prefix(&page.file.path.parent().unwrap()).expect("Couldn't get filename from page asset")),
)?;
}

Expand Down Expand Up @@ -988,7 +988,7 @@ impl Site {
self.copy_asset(
asset_path,
&output_path.join(
asset_path.file_name().expect("Failed to get asset filename for section"),
asset_path.strip_prefix(&section.file.path.parent().unwrap()).expect("Failed to get asset filename for section"),
),
)?;
}
Expand Down