Support filtering glob paths on known extensions #495
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
The current implementation doesn't filter files (it does filter folders though!). What this means is that if a (glob) path contains an unsupported file path, eventually an unsupported file format error will be thrown. By pre-filtering on known image file extensions, we attempt to prevent having a user specify very complex and specific globs. This implementation re-uses "DetermineEncodingFormat.by_extension(ext)" to check whether an extension is known. A disadvantage of this approach is that the above trait methods does slightly more than checking just whether the extension is supported (it also constructs an encoding format). Additionally, now this trait method will be run twice. Once here, and once when determining the format to save it to disk. Perhaps it can be cached in the future? The implementation handles both the extensions supported directly by sic, as well as the extensions as defined by the image crate fallback decider.
…ttern re-use After this, we can start using GlobWalker::from_patterns and add our custom filters to the patterns
foresterre
previously approved these changes
Jul 2, 2020
foresterre
reviewed
Jul 3, 2020
| use globwalk::GlobWalkerBuilder; | ||
| use std::path::PathBuf; | ||
|
|
||
| /// Function to determine the base directory and (remaining) pattern. | ||
| /// Create a generic glob builder | ||
| pub fn glob_builder_base<PAT: AsRef<str>>( |
There was a problem hiding this comment.
I realised something here: we can probably combine this implementation with CommonDir specified elsewhere (they both have the goal of finding the greatest common base directory; here so the smallest amount of paths have to be matched against, and there to find the top level for mirroring the directory structure). For this PR, that doesn't need to be included however, since it was already separately available prior to this PR.
foresterre
approved these changes
Jul 3, 2020
|
Thanks again for the PR btw! |
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.
The current implementation doesn't filter files (it does filter folders though!). What this means is that if a (glob) path contains an unsupported file path, eventually an unsupported file format error will be thrown. By pre-filtering on known image file extensions, we attempt to prevent having a user specify very complex and specific globs.
This implementation re-uses "DetermineEncodingFormat.by_extension(ext)" to check whether an extension is known. A disadvantage of this approach is that the above trait methods does slightly more than checking just whether the extension is supported (it also constructs an encoding format). Additionally, now this trait method will be run twice. Once here, and once when determining the format to save it to disk. Perhaps it can be cached in the future?
The implementation handles both the extensions supported directly by sic, as well as the extensions as defined by the image crate fallback decider.
An alternative idea to the current implementation (this PR) could add specific only known image extensions glob patterns to globwalk iterator to disallow any unsupported image extensions through globwalk. This would limit to filter the walkdir iterator once during globwalk iteration, instead of twice (during globwalk iteration and the in this PR implemented post globwalk filter). I went with the current implementation because I wasn't sure yet how to merge such patterns with the user given pattern.
closes #494