Fix #908: Omit shadowed assets from build #1147
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I Having slept on it, I think the right solution is to prevent the asset tree as a single merged tree containing the directories/files accessible via any of the asset directories. This |
dairiki
force-pushed
the
fix.908-omit-shadowed-assets
branch
3 times, most recently
from
6e4b1a6
to
7f3026f
Compare
dairiki
force-pushed
the
fix.908-omit-shadowed-assets
branch
from
7f3026f
to
aec573e
Compare
When themes are in use, assets can be located in one of multiple asset directories. Previously, in this case, we dealt with multiple asset roots, each with their own heirarchy of descendant assets. Some of these descendants can be shadowed by assets in one of the other asset roots, and should be ignored. This is difficult to manage. Here we refactor the code in `lektor.assets` to present a single logical merged asset tree which overlays the assets from the primary project `assets` directory with the `assets` directories of any themes.
This cleans up the logic for detecting cases where the URL (or asset) name differs from the source file name. (There are cases where the suffixes of the names can differ.) This disuses and deprecates the `from_url` parameter to `Asset.get_child`.
`Asset.build_asset` is an undocumented no-op. It appears to be unreferenced by Lektor itself as well as all Lektor plugins that I've been able to find.
This was breaking the (imperfect) logic in `lektor.builder.PathCache.to_source_file` which checks that source files are contained within the project tree.
Optimization. Since Assets are tied to a Pad, the cache lifetime will match that of the Pad. This seems appropriate.
dairiki
force-pushed
the
fix.908-omit-shadowed-assets
branch
from
aec573e
to
0116d2e
Compare
dairiki
added a commit
to dairiki/lektor
that referenced
this pull request
Sep 11, 2023
* test: add test to exercise lektor#908 * refactor(lektor.assets): present a single merged asset tree When themes are in use, assets can be located in one of multiple asset directories. Previously, in this case, we dealt with multiple asset roots, each with their own heirarchy of descendant assets. Some of these descendants can be shadowed by assets in one of the other asset roots, and should be ignored. This is difficult to manage. Here we refactor the code in `lektor.assets` to present a single logical merged asset tree which overlays the assets from the primary project `assets` directory with the `assets` directories of any themes. * refactor(lektor.assets): be more careful about validating path components * refactor(lektor.assets): refactor Asset.resolve_url_path This cleans up the logic for detecting cases where the URL (or asset) name differs from the source file name. (There are cases where the suffixes of the names can differ.) This disuses and deprecates the `from_url` parameter to `Asset.get_child`. * refactor(lektor.assets)!: remove vestigial method `Asset.build_asset` `Asset.build_asset` is an undocumented no-op. It appears to be unreferenced by Lektor itself as well as all Lektor plugins that I've been able to find. * tests(lektor.assets): enough annotations to get mypy to pass * tests(lektor.assets): increase test coverage * fix: do not Path.resolve() asset roots This was breaking the (imperfect) logic in `lektor.builder.PathCache.to_source_file` which checks that source files are contained within the project tree. * refactor(lektor.assets): cache child assets on Directory Optimization. Since Assets are tied to a Pad, the cache lifetime will match that of the Pad. This seems appropriate.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue(s) Resolved
Fixes #908
Fixes #1111
Related Issues / Links
Description of Changes
The issue described in #908 happens when themes (and so multiple asset trees) are in play. If there is an asset in one asset tree at the same path as an asset in a second asset tree, both are currently built. Besides being inefficient (one overwrites the other) the order of building seems not to be completely deterministic, so that it's not clear which version wins.
Currently, when themes are in play, so that there are multiple on-disk directory trees of assets to be merged to a single output tree, Lektor keeps a list of the multiple assets tree roots, and traverses each of those separately to find all the assets to be built.
In this PR we refactor
lektor.assets.Directoryso that it can represent a logically merged asset directory. After this PR a singlelektor.assets.Directoryinstance can represent an overlayed set of directories. This gets us back to having a single asset root which represents the merged asset tree.