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.

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

Refactor: create_directory responsibly #2407

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Refactor: create_directory responsibly #2407

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
81e5d23
Remove ensure_directory_exists since it's identical to create_directo…
clarfonthey Dec 21, 2023
2da24ab
Don't create directories unless needed; rely on create_dir_all instea…
clarfonthey Dec 21, 2023
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
4 changes: 2 additions & 2 deletions components/imageproc/src/processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ impl Processor {
/// Run the enqueued image operations
pub fn do_process(&mut self) -> Result<()> {
if !self.img_ops.is_empty() {
ufs::ensure_directory_exists(&self.output_dir)?;
ufs::create_directory(&self.output_dir)?;
}

self.img_ops
Expand All @@ -197,7 +197,7 @@ impl Processor {
return Ok(());
}

ufs::ensure_directory_exists(&self.output_dir)?;
ufs::create_directory(&self.output_dir)?;
let output_paths: HashSet<_> = self
.img_ops
.iter()
Expand Down
39 changes: 10 additions & 29 deletions components/site/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use std::time::Instant;
use templates::{load_tera, render_redirect_template};
use utils::fs::{
clean_site_output_folder, copy_directory, copy_file_if_needed, create_directory, create_file,
ensure_directory_exists,
};
use utils::net::{get_available_port, is_external_link};
use utils::templates::{render_template, ShortcodeDefinition};
Expand Down Expand Up @@ -639,18 +638,13 @@ impl Site {
create_dirs: bool,
) -> Result<PathBuf> {
let write_dirs = self.build_mode == BuildMode::Disk || create_dirs;
ensure_directory_exists(&self.output_path)?;

let mut site_path = RelativePathBuf::new();
let mut current_path = self.output_path.to_path_buf();

for component in components {
current_path.push(component);
site_path.push(component);

if !current_path.exists() && write_dirs {
create_directory(&current_path)?;
}
}

if write_dirs {
Expand Down Expand Up @@ -788,9 +782,13 @@ impl Site {
}

pub fn render_themes_css(&self) -> Result<()> {
ensure_directory_exists(&self.static_path)?;
let themes = &self.config.markdown.highlight_themes_css;

if !themes.is_empty() {
create_directory(&self.static_path)?;
}

for t in &self.config.markdown.highlight_themes_css {
for t in themes {
let p = self.static_path.join(&t.filename);
if !p.exists() {
let content = &self.config.markdown.export_theme_css(&t.theme)?;
Expand Down Expand Up @@ -818,7 +816,7 @@ impl Site {
}

pub fn build_search_index(&self) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
create_directory(&self.output_path)?;
// TODO: add those to the SITE_CONTENT map

// index first
Expand Down Expand Up @@ -857,7 +855,6 @@ impl Site {
/// Renders all the aliases for each page/section: a magic HTML template that redirects to
/// the canonical one
pub fn render_aliases(&self) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
let library = self.library.read().unwrap();
for (_, page) in &library.pages {
for alias in &page.meta.aliases {
Expand All @@ -874,7 +871,6 @@ impl Site {

/// Renders 404.html
pub fn render_404(&self) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
let mut context = Context::new();
context.insert("config", &self.config.serialize(&self.config.default_language));
context.insert("lang", &self.config.default_language);
Expand All @@ -886,7 +882,6 @@ impl Site {

/// Renders robots.txt
pub fn render_robots(&self) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
let mut context = Context::new();
context.insert("config", &self.config.serialize(&self.config.default_language));
let content = render_template("robots.txt", &self.tera, context, &self.config.theme)?;
Expand All @@ -911,8 +906,6 @@ impl Site {
return Ok(());
}

ensure_directory_exists(&self.output_path)?;

let mut components = Vec::new();
if taxonomy.lang != self.config.default_language {
components.push(taxonomy.lang.as_ref());
Expand Down Expand Up @@ -977,8 +970,6 @@ impl Site {

/// What it says on the tin
pub fn render_sitemap(&self) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather keep those around even if in theory it's already created elsewhere

Copy link
Contributor Author

@clarfonthey clarfonthey Jan 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The reason why I removed it here is because write_content always creates directories when called, so, you can rely on that instead of creating them ahead of time.

This is why the only remaining instances where directories created are there, and when copying assets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's ok in the grand scheme of things but creates inter-dependencies in case we decide to not use write_content for some reason. Maybe it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. My thought process was that we should delay creating the directory as long as possible to avoid making them unnecessarily, but I'm open to adding back in checks that are guarded on whether things will actually be written.


let library = self.library.read().unwrap();
let all_sitemap_entries =
{ sitemap::find_entries(&library, &self.taxonomies[..], &self.config) };
Expand Down Expand Up @@ -1032,8 +1023,6 @@ impl Site {
lang: &str,
additional_context_fn: impl Fn(Context) -> Context,
) -> Result<()> {
ensure_directory_exists(&self.output_path)?;

let feed = match feed::render_feed(self, all_pages, lang, base_path, additional_context_fn)?
{
Some(v) => v,
Expand All @@ -1060,27 +1049,22 @@ impl Site {

/// Renders a single section
pub fn render_section(&self, section: &Section, render_pages: bool) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
let mut output_path = self.output_path.clone();
let mut components: Vec<&str> = Vec::new();
let create_directories = self.build_mode == BuildMode::Disk || !section.assets.is_empty();

if section.lang != self.config.default_language {
components.push(&section.lang);
output_path.push(&section.lang);

if !output_path.exists() && create_directories {
create_directory(&output_path)?;
}
}

for component in &section.file.components {
components.push(component);
output_path.push(component);
}

if !output_path.exists() && create_directories {
create_directory(&output_path)?;
}
if create_directories {
create_directory(&output_path)?;
}

if section.meta.generate_feed {
Expand Down Expand Up @@ -1166,7 +1150,6 @@ impl Site {

/// Renders all pages that do not belong to any sections
pub fn render_orphan_pages(&self) -> Result<()> {
ensure_directory_exists(&self.output_path)?;
let library = self.library.read().unwrap();
for page in library.get_all_orphan_pages() {
self.render_page(page)?;
Expand All @@ -1181,8 +1164,6 @@ impl Site {
components: Vec<&'a str>,
paginator: &'a Paginator,
) -> Result<()> {
ensure_directory_exists(&self.output_path)?;

let index_components = components.clone();

paginator
Expand Down
4 changes: 2 additions & 2 deletions components/site/src/sass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ use libs::walkdir::{DirEntry, WalkDir};

use crate::anyhow;
use errors::{bail, Result};
use utils::fs::{create_file, ensure_directory_exists};
use utils::fs::{create_directory, create_file};

pub fn compile_sass(base_path: &Path, output_path: &Path) -> Result<()> {
ensure_directory_exists(output_path)?;
create_directory(output_path)?;

let sass_path = {
let mut sass_path = PathBuf::from(base_path);
Expand Down
8 changes: 0 additions & 8 deletions components/utils/src/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,6 @@ pub fn create_file(path: &Path, content: &str) -> Result<()> {
Ok(())
}

/// Create a directory at the given path if it doesn't exist already
pub fn ensure_directory_exists(path: &Path) -> Result<()> {
if !path.exists() {
create_directory(path)?;
}
Ok(())
}

/// Very similar to `create_dir` from the std except it checks if the folder
/// exists before creating it
pub fn create_directory(path: &Path) -> Result<()> {
Expand Down