-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(core): add AssetManager::iter #8288
Conversation
This new function allows users to iterate on all embedded assets, important if you want to AssetManager::get an asset you are not sure exists.
@@ -163,6 +166,10 @@ impl Assets for EmbeddedAssets { | |||
.map(|a| Cow::Owned(a.to_vec())) | |||
} | |||
|
|||
fn iter(&self) -> Box<dyn Iterator<Item = (&&str, &&[u8])> + '_> { | |||
Box::new(self.assets.into_iter()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the box here? can't we implement InotIterator
directly on the type and it construct a new type AssetEntries
that is an iterator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to be dyn
because we're in the context of a trait.. so it needs Box too to be sized. Writing another type doesn't help this. It's also the solution used for csp_hashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we return an AssetsEntries
struct, we don't need to deal with dyn
and can just implement Iterator
on AssetsEntries
fn iter<'a>(&'a self) -> AssetsEntries<'a, &'_ str, &'_ [u8]>;
or we can return phf::map::Entries
directly
fn iter<'a>(&'a self) -> phf::map::Entries<'a, &'_ str, &'_ [u8]>;
This should also be done for csp_hashes
as well but to avoid breaking changes, we can delay that change to v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but then the AssetEntries need to hold the iterator anyway, unless we force the type to be a HashMap which isn't a good idea really for a trait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only downside I see with this, is that AssetsEntries
will need to use phf::map::Entries
as a concrete type under the hood and now the trait Assets
is not so generic and that makes me wonder, do we need this trait? seems like our usage of it, could be just replaced with the concrete EmbeddedAssets
type
edit: GitHub didn't show me your previous message about the HashMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for tests it's nice to have this trait
but we can also put the author @chippers on the line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a blocker btw, just would be a nice improvement for v2 (unless the trait is really required)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late on seeing this @amrbashir @lucasfernog-crabnebula , the asset trait allows people to implement their own asset handling, such as if they want to fetch them from disk or network. it's one of the original reasons it became possible to change the items in a Context
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___
)Other information
This new function allows users to iterate on all embedded assets, important if you want to AssetManager::get an asset you are not sure exists.