-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(core): add AssetManager::iter #8288
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
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.
| } | ||
|
|
||
| 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.