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

Refactor: create_directory responsibly #2407

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

clarfonthey
Copy link
Contributor

These are the first two commits of #2388 separated into their own PR. Specifically:

  • At some point, ensure_directory_exists was made identical to create_directory. I removed the former and replaced all instances with the latter.
  • A lot of methods were running create_directory iteratively on every component despite it using create_dir_all internally. I decided to update these to have fewer create_directory calls so they could be better tracked.

@clarfonthey clarfonthey mentioned this pull request Jan 10, 2024
3 tasks
@@ -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

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.

@Keats Keats merged commit a6bd2b9 into getzola:next Jan 18, 2024
5 checks passed
@clarfonthey clarfonthey deleted the create-directory branch January 18, 2024 17:31
veluca93 pushed a commit to veluca93/zola that referenced this pull request May 14, 2024
* Remove ensure_directory_exists since it's identical to create_directory, and misleading

* Don't create directories unless needed; rely on create_dir_all instead of manually iterating over components
Keats pushed a commit that referenced this pull request Jun 20, 2024
* Remove ensure_directory_exists since it's identical to create_directory, and misleading

* Don't create directories unless needed; rely on create_dir_all instead of manually iterating over components
berdandy pushed a commit to berdandy/azola that referenced this pull request Sep 17, 2024
* Remove ensure_directory_exists since it's identical to create_directory, and misleading

* Don't create directories unless needed; rely on create_dir_all instead of manually iterating over components
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants