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

Add is_dir helper function to metadata #193

Merged
merged 8 commits into from
Nov 22, 2022
Merged

Conversation

jaztec
Copy link
Contributor

@jaztec jaztec commented Nov 19, 2022

Jumping on #170, adding an is_dir helper function based on fs metadata.

@jaztec
Copy link
Contributor Author

jaztec commented Nov 19, 2022

@pyrossh
Copy link
Owner

pyrossh commented Nov 20, 2022

LGTM

@jaztec
Copy link
Contributor Author

jaztec commented Nov 20, 2022

I've been writing some tests and noticed it is not possible to fetch a folder anyway. That does make this PR a tad useless.

I could extend https://github.com/pyrossh/rust-embed/blob/master/utils/src/lib.rs#L111 a bit to also read a folder but I'm not sure what behavior should be displayed and/or what usecase it would fullfill.

@pyrossh
Copy link
Owner

pyrossh commented Nov 20, 2022

Yes I was wondering that. I thought it could be useful to do some recursive stuff. Or maybe get a default index.html if the path is folder. Example use case would be something like hosting a website using a static site generator like hugo.

@jaztec
Copy link
Contributor Author

jaztec commented Nov 20, 2022

I updated the function, it will now read the directory metadata but keeps a clear Vec<u8> for data, hash will be empty as well.

@jaztec
Copy link
Contributor Author

jaztec commented Nov 20, 2022

Yes I was wondering that. I thought it could be useful to do some recursive stuff. Or maybe get a default index.html if the path is folder. Example use case would be something like hosting a website using a static site generator like hugo.

Fetching an index.html would be interesting as well. Altough it might a bit "magical" what happens under the hood, maybe adding a default_file function could make the behavior and intend more clear?

@pyrossh
Copy link
Owner

pyrossh commented Nov 20, 2022

I would leave that upto the user to implement it in the web framework of their choice, as its a specific scenario that only applies to JAM stack based websites.

@pyrossh pyrossh merged commit 0006648 into pyrossh:master Nov 22, 2022
@pyrossh
Copy link
Owner

pyrossh commented Nov 22, 2022

Thanks @jaztec

@jaztec
Copy link
Contributor Author

jaztec commented Nov 23, 2022

I fixed the BC break issue by the way, I scr*wed up big time there. jaztec@13fabb9

However the branch still doesn't add any real value. I would like to extend a next PR to actually include folders as new root elements. So you can use something like let new_root = Asset::get('path/to/folder'); root.get('file.html').

So it can be an actual BC breaking version bump.

@pyrossh
Copy link
Owner

pyrossh commented Nov 24, 2022

I guess instead of overriding the Asset::get function you could have another function Asset::folder which returns the instance of the folder. But do you have any use cases for this? I haven't see this used in the wild though.

@jaztec
Copy link
Contributor Author

jaztec commented Nov 24, 2022

I guess instead of overriding the Asset::get function you could have another function Asset::folder which returns the instance of the folder

True, that would not cause a break.

But do you have any use cases for this? I haven't see this used in the wild though.

I was thinking of a usecase I once build in Go, it was a project which used a rule engine to handle incoming requests based on hostname and path. It would either proxy a placeholder, redirect to a marketing site or dynamically load a SPA website if it was present.

But thinking again I recon the entire purpose of the library is loading from embedded files, thus files known at compile time. When I got the assets at compile time there is no need anymore to load them dynamically I guess.

You want me to make a safe PR for the is_dir helper? The iter will only return actual files, only the get will return a EmbeddedFile with an optional is_dir: true.

@AzureMarker
Copy link
Collaborator

I guess instead of overriding the Asset::get function you could have another function Asset::folder which returns the instance of the folder

True, that would not cause a break.

This is still a breaking change unless the new trait function has a default implementation. It's possible for users to implement RustEmbed on their own types (ex. to mock it).

@jaztec
Copy link
Contributor Author

jaztec commented Nov 24, 2022

This is still a breaking change unless the new trait function has a default implementation. It's possible for users to implement RustEmbed on their own types (ex. to mock it).

True again, from a different participator this time. I actually don't think having dynamic folders is usefull anymore, just because my last comment as a whole. At compile time you are aware of the folders included and so you can embed them at your own discretion. Having it be loaded dynamically doesn't add any value.

My only question now is; do I submit a new PR for this

You want me to make a safe PR for the is_dir helper? The iter will only return actual files, only the get will return a EmbeddedFile with an optional is_dir: true.

Based on this: jaztec@13fabb9

@AzureMarker
Copy link
Collaborator

I think we need to understand the use case behind #170 before doing any implementation.

@jaztec
Copy link
Contributor Author

jaztec commented Nov 24, 2022

Let's open it up then. I can't so be my guest

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.

None yet

3 participants