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

Cannot enumerate multiple times with version 1.1.7 #59

Closed
Indigo744 opened this issue Jul 16, 2020 · 12 comments · Fixed by #60
Closed

Cannot enumerate multiple times with version 1.1.7 #59

Indigo744 opened this issue Jul 16, 2020 · 12 comments · Fixed by #60

Comments

@Indigo744
Copy link

Hello,

First of all, thanks for this amazing lib.

However, after upgrading to 1.1.7, which seemed to be a minor revision, our code did not work anymore.

At first, I couldn't find the corresponding release: it stops at 1.1.5.
Then I found the changelog and saw these information:
image

First of all, changing the way the lib works by default is a breaking change, and warrant a major revision (as stated by semantic versioning)...

In any case, I went into our code and looked for changing the options passed to the globbing lib, and to my dismay I didn't find the GlobOptions.MatchFilenameOnly, even though I had the 1.1.7 Nuget:
image

Even weirder, this is not the same with the https://github.com/kthompson/glob/blob/master/src/Glob/GlobOptions.cs:
image

And even Github have trouble determining the latest commit date but the file history reveals that the 6948bac commit was committed on Mar 11, 2020.

So in the end, I am not really sure what is happening with this seemingly minor-maybe-major release that I can't use.

Thank you for your insight.

@kthompson
Copy link
Owner

kthompson commented Jul 16, 2020

Looks like I made a typo in the ChangeLog.md. The MatchFilenameOnly flag should not be in version 1.1.7. Is this what you are seeing?

The ChangeLog has been updated. Can you provide some details as to how your code "didnt work anymore"?

Also, to clarify, 1.1.7 should not have breaking changes, the "Changed" section in 1.1.7 is still unreleased and not part of any existing release.

@Indigo744
Copy link
Author

Ah indeed, the changelog makes more sense now.

However, we are still having issue with the latest 1.1.7.

After playing with the code a while, I found the issue.
Here is the minimal code:

// Create file in current folder
File.WriteAllText("./test.txt", "test");

// Search for all files
var lst = (new DirectoryInfo(@".")).GlobFiles(@"**/*.txt");
lst.ToList().ForEach(f => Console.WriteLine(f.FullName));
lst.ToList().ForEach(f => Console.WriteLine(f.FullName));

With Glob 1.1.6, we see test.txt twice.
Proof: https://dotnetfiddle.net/5dwmr4
With Glob 1.1.7, we see test.txt only once!
Proof: https://dotnetfiddle.net/p7ojwo

The problem does not exists with a simpler pattern *.txt, so I suspect the issue comes from the any folder pattern (**).
The problem does not exists if I add a .ToList() directly on the variable lst before using it twice, so I suspect the issue is tied to the IEnumerable type (maybe a yield is misplaced?).

Hope it helps. Thank you!

@Indigo744
Copy link
Author

After looking at the changelog and the related bug fixed #52 I think what we see is the direct result of the fix.

Files matching a pattern in multiple ways are only emitted once

Before, we saw the file twice because it was matched with *.txt and **\*.txt.
With the fix, we see it only once.

So I guess the issue was more in our code, where we still used the IEnumerable interface instead of converting it to a proper list.
And I can see in your example in the Readme file, you use .ToArray(); to get the full list. So we really should have been more attentive.

If you agree, close this issue and accept my apology for wasting your time.

@ejball
Copy link
Contributor

ejball commented Jul 18, 2020

I agree that this is a bug. Enumerating the same enumerable multiple times should give the same results each time.

@kthompson
Copy link
Owner

Thanks for the clarification, I will take a look at the double enumeration. It looks like the second enumeration uses the cache still so nothing is emitted. I need to do a bit more investigation to determine the cause though.

@Indigo744
Copy link
Author

@kthompson take your time, as doing a .ToList() resolves the issue for us.

@robertcoltheart
Copy link

To add to this, if you create 2 files, 1 in a deeper directory, the glob won't pick up the second file. Tested under Linux.

eg.

File.WriteAllText("test1.txt", "test");
File.WriteAllText("deep/test2.txt", "test");

// Search for all files
Glob.Files(Environment.CurrentDirectory, "**/*.txt")

If only test1.txt exists, it will pick up this file. If the test.2.txt file is added, only this file will be picked up.

@kthompson kthompson changed the title Issue with latest version 1.1.7 Cannot enumerate multiple tiles with version 1.1.7 Aug 7, 2020
@kthompson kthompson changed the title Cannot enumerate multiple tiles with version 1.1.7 Cannot enumerate multiple times with version 1.1.7 Aug 7, 2020
kthompson added a commit that referenced this issue Aug 7, 2020
The previous change caused multiple enumerations to return different results

Closes #59
@kthompson
Copy link
Owner

@ejball @Indigo744 @robertcoltheart I put up a new build that should address the issues. If you would like to provide any input on them you can take a look here: https://automaters.visualstudio.com/Glob/_build/results?buildId=289&view=artifacts&type=publishedArtifacts

If it looks good I'll merge and close this issue.

@robertcoltheart
Copy link

It may have addressed the original issue, but my issue (documented above) still remains with the 1.1.8 beta version. Happy to open a separate issue if you want to move forward.

@kthompson
Copy link
Owner

It may have addressed the original issue, but my issue (documented above) still remains with the 1.1.8 beta version. Happy to open a separate issue if you want to move forward.

Yea, let's open a separate issue. I was not able to reproduce your specific issue I also added a test based on your description and it passes so if there is something I missed let me know.

@robertcoltheart
Copy link

Ok, I think you can ignore me, this was a schoolboy Linux error. I was running a command line app where I could pass in a glob pattern to use, but I wasn't using "" to enclose the pattern and Linux was exploding the glob pattern for me instead of passing it to your library. Sorry if you spent any time on this.

@kthompson
Copy link
Owner

This is fixed in 1.1.8 https://www.nuget.org/packages/Glob/1.1.8

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 a pull request may close this issue.

4 participants