Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Use globwalk instead of glob #95

Merged
merged 1 commit into from
Oct 7, 2018
Merged

Use globwalk instead of glob #95

merged 1 commit into from
Oct 7, 2018

Conversation

h-michael
Copy link
Contributor

implementation of #74 .

Now, we can use {a, b} pattern.

@h-michael
Copy link
Contributor Author

h-michael commented Oct 7, 2018

@killercup
Globwalk find directories recursively only specified max_depth becauseof use WalkDir crate.
So I think that using globwalk instead of glob is not good.
How do you think that?

@killercup
Copy link
Owner

Having a max depth is fine with me. Maybe you can document this, though?

Also, if you rebase on master, CI should be fixed.

@h-michael
Copy link
Contributor Author

Having a max depth is fine with me.

Should we change glob function argument from (patterns: &str) to (patterns: &str, min_depth: Option, max_depth: Option)`

@killercup
Copy link
Owner

Should we change glob function argument from (patterns: &str) to (patterns: &str, min_depth: Option, max_depth: Option)

Nah, quicli is about giving you simple functions. Let's just use the defaults for now and add comment saying "If you need to work with very deeply nested directories, please use the globwalk crate directly."

@killercup
Copy link
Owner

Ah, CI is green already. Thanks for contributing this!

@killercup killercup merged commit 31862d2 into killercup:master Oct 7, 2018
@h-michael h-michael deleted the globwalk branch October 7, 2018 15:16
@h-michael
Copy link
Contributor Author

@killercup

Let's just use the defaults for now.

I'm sorry, I was not successfully communicated., default max_depth is not set.
If we use default, *.md finds recursively with none limit.
If we use glob instead of globwalk, *.md finds none recursively.
But **/*.md is finds recursively with none limit.

@killercup
Copy link
Owner

I'm not sure I understand? I'd expect *.md to only check for files ending with .md in the current directory. **/*.md I expect to find markdown files in subdirectories up -- up to the limit you describe.

What are you proposing we do?

@h-michael
Copy link
Contributor Author

@killercup

I am sorry that I was not good at English, so if I did not convey it to you.
I mean that this PR contains a Breaking change.
I will explain it using existing tests.

This test is not changed my PR.
https://github.com/killercup/quicli/blob/master/src/fs.rs#L91

But if we added test case like this, using glob could pass this but using globwalk that is my commit couldn't pass this case because of limitting max_depth.

/// let markdown_files = glob("**/*.md")?;
/// assert_eq!(markdown_files.len(), 5);

So if I tried to remove max_depth method, it could pass above case.
But it could not this case.
This case gets 5.

/// let markdown_files = glob(*.md")?;
/// assert_eq!(markdown_files.len(), 2);

Did you recognize this breaking change?
If you don't recognize this, how do you think about this?

Sorry for my poor English.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants