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

Implementing node-glob's ignore option, fixes #24 #40

Closed
wants to merge 5 commits into from

Conversation

UltCombo
Copy link
Contributor

Quite a few tests are broken on my machine, not sure if I'm missing something or the ignore option is buggy in regard to the dot option and absolute paths (on Windows).

Fixes #24

Quite a few tests are broken on my machine, not sure if I'm missing something or the `ignore` option is buggy in regard to the `dot` option and absolute paths (on Windows).
@yocontra
Copy link
Member

Actually if it cleans up the code lets go ahead and remove regexp support, I've never seen anyone use it. No need to keep that feature - it isn't even documented well afaik

@UltCombo
Copy link
Contributor Author

Something odd is that Mocha was seemingly "cascading" certain test fails -- e.g. in my first commit above, the "glob-stream create() should handle RegExps as negative matchers" test shows up as a fail, but it passes when run in isolation.

if (negatives.length) {
// Do not mutate the options object which may be used
// in multiple `gs.createStream()` calls.
opt = assign({}, opt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldnt a lodash.clone be better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use object-assign as its logic is rather simple and well-spec'd (follows ECMAScript 2015 spec.). But yes, I guess a clone or cloneDeep would suffice as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add the entire lodash lib as a dependency or just the clone method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@UltCombo
Copy link
Contributor Author

I've removed RegExp support, but no change in the failed tests. I wonder if it is an issue with glob's ignore option, or if I'm just too sleepy to see an obvious error.

globArray.push({
index: index,
glob: glob
glob: globArray === negatives ? glob.slice(1) : glob
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should just be isNegative(glob) ? glob.slice(1) : glob then ditch the globArray var

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the globArray.push above? Should I inline (isNegative(glob) ? glob.slice(1) : glob).push() as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would put isNegative in a var so it only gets called once then ternary the other two places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@UltCombo
Copy link
Contributor Author

Oh -- I'm forgetting to unrelative the negative globs, doh.

@UltCombo
Copy link
Contributor Author

Looks like there is still an issue with unrelative logic.

@UltCombo
Copy link
Contributor Author

Oh wait, the unrelative logic is fine -- node-glob's ignore option seems to be screwing it up independent of using relative or absolute paths.

@UltCombo UltCombo changed the title Implementing node-glob's ignore option Implementing node-glob's ignore option, fixes #24 Feb 24, 2015
@UltCombo
Copy link
Contributor Author

"should return a file name stream with dotfiles negated" is failing due to a node-glob issue.

@UltCombo
Copy link
Contributor Author

I thought the error could be due to passing absolute paths (which use \ as dir separator in Windows) to the ignore option (according to the docs, node-glob always interprets \ as escape sequences).

Minimatch@2 does treat \ as a valid dir separator, but node-glob currently does not.

The test is still failing on Travis (Linux), so there must be yet another error...

@yocontra
Copy link
Member

Any progress on this?

@UltCombo
Copy link
Contributor Author

@contra there are some blocking issues:

@UltCombo
Copy link
Contributor Author

@contra If you don't mind the breaking change regarding the dot behavior, we can skip those tests temporarily and I can try fixing the remaining logic again.

@yocontra
Copy link
Member

@UltCombo Seems like the resolution from the glob issue was the ball is in your court. Do you need any help with this?

isaacs/node-glob#166 (comment)

@UltCombo
Copy link
Contributor Author

@contra Well yes, I have near to no experience with the node-glob code base, and a good part of it seems nearly unreadable to me.

@thirdcreed
Copy link
Contributor

I nearly have a working PR for this, there's one test broken:
'should return a file name stream with dotfiles negated'
and it's timing out at 2000ms.

Any thoughts on what that might be, for some reason the stream is not emitting anything.
https://github.com/thirdcreed/glob-stream/blob/master/index.js

@thirdcreed
Copy link
Contributor

isaacs/node-glob#232 This is a block for me. I think there's some bigger problem involving globs that end with "/**", a completely different matcher is used for those cases. There's some kind of race condition. It emits before the on end event is registered.

@thirdcreed
Copy link
Contributor

I'm going to wait until issacs has time to answer it, I guess this above my head. Apparently the race condition thing is understood, It looks like its on the user to guarantee their events. Unless issacs has some insight on why their would be a race condition on the this one end event, in this one edge case. I’m out of ideas. I've been hacking on my own node-glob, but I can't seem to get that one fucking test to run.

It looks like @UltCombo's on this again anyway.

@UltCombo
Copy link
Contributor Author

There are 23 failing tests (on both current master and @thirdcreed's branch) when testing on Windows. This does not make it easy to work on a fix and I'm relatively short on free time.

I guess I can give this another shot on my Linux or Mac box soon.

@contra May I suggest adding AppVeyor (Windows CI)? Here's a sample appveyor.yml just in case.

@yocontra
Copy link
Member

@UltCombo We use appveyor on some other stuff, feel free to add it if you need it

@UltCombo
Copy link
Contributor Author

@contra Do you have an appveyor config of your choice to add to this repository? Or should I just create one using the same Node.js versions as this repo's Travis config?

@phated
Copy link
Member

phated commented Nov 27, 2015

@jonschlinkert
Copy link
Contributor

Something odd is that Mocha was seemingly "cascading" certain test fails

if there are multiple tests with the same description, mocha will run them all, even if one has .only on it. I remember this driving me crazy a few times before @doowb noticed what was happening and pointed it out. again, not sure if it's related. but if not, maybe it will save someone else from going crazy lol.

@phated
Copy link
Member

phated commented Sep 7, 2016

Closing in favor of #71 which is passing all the tests. Thanks for putting in a lot of the work here @UltCombo

@phated phated closed this Sep 7, 2016
@UltCombo
Copy link
Contributor Author

UltCombo commented Sep 8, 2016

Oh, this was still on my TODO list, but kept falling off the radar.

No problem, and indeed, #71 seems more suitable and up-to-date. I'll comment there if I find anything odd.

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.

Using src globs can be really slow with negation
5 participants