feature/traverse: Add an option to rely purely on ignore files for traversing directories#339
feature/traverse: Add an option to rely purely on ignore files for traversing directories#339MA-DDN wants to merge 10 commits intodrahnr:masterfrom
Conversation
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
With some delay, I am not opposed to the changes, yet with the added complexity some more code unification and carve outs need to be done before I'll merge this.
Sorry for the long review delay.
|
Gentle ping @MA-DDN |
MA-DDN
force-pushed
the
add-basic-gitignore-support
branch
from
7765ad5 to
99a20df
Compare
|
Looks like this was updated. Could it be re-reviewed? |
|
I'll get to it next week |
drahnr
left a comment
drahnr
left a comment
There was a problem hiding this comment.
Hey, thank you for the PR and the work you put in, it looks good on a technical level, bar the comments I left.
I have a larger scope question: What issue are you solving with using only .gitignore driven file selection? Can you elaborate the use-case, both as part of a comment here and in the relevant code pieces?
| /// This flag also modifies the behaviour of the recursive flag | ||
| /// to purely recurse down directories. | ||
| #[clap(short, long)] | ||
| pub gitignore: bool, |
There was a problem hiding this comment.
I think this carries ambiguous meaning for the user. We should find a name that includes either follow or only or both terms to convey the meaning you want.
| /// This flag also modifies the behaviour of the recursive flag | ||
| /// to purely recurse down directories. | ||
| #[clap(short, long)] | ||
| gitignore: bool, |
There was a problem hiding this comment.
I don't think we need this twice, do we?
| ); | ||
| continue; | ||
| } | ||
| /// Process a path that could be a file or directory, with optional directory traversal logic |
There was a problem hiding this comment.
Requires documentation how Option::None is to be interpreted or use of a custom enum. The previous version, while more complex, avoided that :)
| let cwd = cwd()?; | ||
|
|
||
| // Collect files using gitignore rules | ||
| let collect_gitignore_files = |flow: &mut VecDeque<PathBuf>, |
There was a problem hiding this comment.
It doesn't seem the closure uses any local state, can we upgrade it to a full function?
| .map(|x| x.path().to_owned()) | ||
| }) | ||
| .filter(|path: &PathBuf| { | ||
| path.to_str() |
There was a problem hiding this comment.
we should use path.extension() which yields an OsStr which implements ParitialEq with str or use a match block / matches! invocation to avoid some more visual complexity
| entry | ||
| .ok() | ||
| .filter(|entry| entry.file_type().map(|ft| ft.is_file()).unwrap_or(false)) | ||
| .map(|x| x.path().to_owned()) |
There was a problem hiding this comment.
Retaining the entry and having this one as the last step seems easier to read. Try to avoid |x| x.., use entry to at least leave a crumb of information, particularly in lengthy chains.
| }; | ||
| files_to_check.push(x); | ||
| log::debug!("Processing {} -> {}", path_in.display(), path.display()); | ||
| path.canonicalize().ok() |
There was a problem hiding this comment.
I am always unsure we want to be so lenient here, we should at the very least print a warning if we omit a file.
|
|
||
| // stage 2 - check for manifest, .rs , .md files and directories | ||
| if gitignore { | ||
| let mut all_files = collect_gitignore_files(&mut flow, recurse); |
There was a problem hiding this comment.
We should pass flow by value, we don't use it everafter.
| let mut files_to_check = Vec::with_capacity(64); | ||
|
|
||
| // stage 2 - check for manifest, .rs , .md files and directories | ||
| if gitignore { |
3 participants
What does this PR accomplish?
Changes proposed by this PR:
This pr adds an option flag to change the behaviour of the traversal module to collect all relevant files except those which match an entry in a .gitignore file. Our usecase for this is to be able to ignore certain temporary Markdown files which are pulled in when building our documentation.
📜 Checklist
./demosub directory