Support colocating subfolders #1582
Conversation
| // `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). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah, that's what I meant.
| /// 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) | ||
| } | ||
|
|
There was a problem hiding this comment.
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?
timakro
force-pushed
the
pr-colocation-with-subfolders
branch
from
44d295d to
bb41f78
Compare
|
I've missed some tests which are failing for obvious reasons now. (I don't know how to run all tests combined instead of each component separately.) Before resolving this let's first talk about the proposed change itself: if links to colocated files should be left relative or converted to absolute links. |
|
@Keats I'm worried you're holding off reviewing this because the checks are failing. Please take a look so we can decide on my proposed change. I will either undo my proposed change or adjust the unit tests then. |
I will have a look after 0.14.1, just focusing on fixing bugs rather than new features first. That and also holidays. |
|
Can you rebase your commit on the latest next branch? |
timakro
force-pushed
the
pr-colocation-with-subfolders
branch
from
bb41f78 to
523f367
Compare
|
@Keats Done |
|
With the proposed change two tests would have to be adjusted 1890e38. Still not sure if I'm missing something. |
| // `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). |
There was a problem hiding this comment.
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
|
Amazing, this is the one feature I needed to migrate a site I'm working on to Zola 🎉 |
|
Hey @timakro , unfortounately this pull request seems to break a template of mine. It makes use of the child-page’s assets which cannot be rendered because their path is relative to the child page instead of the section. Could you please have a look? The full discussion is in the forum: https://zola.discourse.group/t/assets-from-child-page-content-have-wrong-relative-paths-when-shown-in-sections/1181/ |
This check was removed in getzola#1582 but broke using page assets in their parent section. I re-introduced them in a way that should still work with co-located subdirectories. This commit is breaking a test however, which I don't yet understand how. Further info: - getzola#1582 - https://zola.discourse.group/t/assets-from-child-page-content-have-wrong-relative-paths-when-shown-in-sections/1181/6
In getzola#1582 I broke the asset colocation example from the documentation: https://www.getzola.org/documentation/content/overview/#asset-colocation The breakage was discovered by irimi1: getzola#1582 (comment) When I wrote getzola#1582 I was not aware of the old behavior: links to assets used to be rewritten to absolute links, so that colocated assets could be linked to from `pagename/index.md` simply as `asset.xyz` instead of `pagename/asset.xyz`. I unintentionally changed this, so that currently you have to write `pagename/asset.xyz`. Read this commit as a partial revert of getzola#1582. It restores the old behavior as described above. Caveats: 1. This will break lots of sites, just like getzola#1582 unintentionally broke lots of sites (but no one seemed to notice). 2. The old behavior is restored, including the quirk, that when linking from a page `pagename.md` to a section asset `asset.xyz` you need to write `../asset.xyz`, even though both files are in the same folder on disk. You can make sense of it in the following way: the asset belongs to the section so that is one level up from the page.
Restores the change of relative to absolute links for colocated files that was removed in commit 4086b07 / PR getzola#1582.
Restores change of relative links to absolute links of colocated files removed by commit 4086b07 / PR getzola#1582.
* Support colocating subfolders * Adjust tests
Closes #894.