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

Consider updating is-glob, glob-parent to 3.x series #3

Open
pravi opened this issue Oct 28, 2016 · 17 comments
Open

Consider updating is-glob, glob-parent to 3.x series #3

pravi opened this issue Oct 28, 2016 · 17 comments

Comments

@pravi
Copy link

pravi commented Oct 28, 2016

When running tests with latest versions of glob-parent and is-glob,

git diff
diff --git a/package.json b/package.json
index 8cb2dbf..fe4d7cc 100644
--- a/package.json
+++ b/package.json
@@ -29,8 +29,8 @@
     "test": "mocha"
   },
   "dependencies": {
-    "glob-parent": "^2.0.0",
-    "is-glob": "^2.0.0"
+    "glob-parent": "^3.0.0",
+    "is-glob": "^3.0.0"
   },
   "devDependencies": {
     "mocha": "*",

5/10 tests fail, see error below. In debian, we like to keep the latest versions of any library and we already have glob-parent and is-glob at version 3.0.

$ npm test

> [email protected] test /home/pravi/forge/debian/git/pkg-javascript/node-glob-base
> mocha



  glob-base:
    1) typical globs:
    ✓ file extensions:
    2) negation pattern:
    ✓ extglobs:
    3) braces: no base:
    4) braces in filename:
    5) braces in dirname:
    ✓ regex character classes:
    ✓ qmarks:
    ✓ non-glob pattern:


  5 passing (46ms)
  5 failing

  1) glob-base: typical globs::

      AssertionError: expected { base: './{a/b/{c,', glob: 'foo.js}/e.f.g}', isGlob: true } to equal { base: '.', glob: '{a/b/{c,/foo.js}/e.f.g}', isGlob: true } (at base, A has './{a/b/{c,' and B has '.')
      + expected - actual

       {
      -  "base": "./{a/b/{c,"
      -  "glob": "foo.js}/e.f.g}"
      +  "base": "."
      +  "glob": "{a/b/{c,/foo.js}/e.f.g}"
         "isGlob": true
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:38:50)

  2) glob-base: negation pattern::

      AssertionError: expected { base: 'a/b/c', glob: '!foo', isGlob: false } to equal { base: 'a/b/c', glob: '!foo', isGlob: true } (at isGlob, A has false and B has true)
      + expected - actual

       {
         "base": "a/b/c"
         "glob": "!foo"
      -  "isGlob": false
      +  "isGlob": true
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:76:35)

  3) glob-base: braces: no base::

      AssertionError: expected { base: '/a/b/{c,', glob: 'foo.js}/e.f.g/', isGlob: true } to equal { base: '/a/b', glob: '{c,/foo.js}/e.f.g/', isGlob: true } (at base, A has '/a/b/{c,' and B has '/a/b')
      + expected - actual

       {
      -  "base": "/a/b/{c,"
      -  "glob": "foo.js}/e.f.g/"
      +  "base": "/a/b"
      +  "glob": "{c,/foo.js}/e.f.g/"
         "isGlob": true
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:87:48)

  4) glob-base: braces in filename::

      AssertionError: expected { base: 'a/b/.{c,', glob: '.gitignore}', isGlob: true } to equal { base: 'a/b', glob: '.{c,/.gitignore}', isGlob: true } (at base, A has 'a/b/.{c,' and B has 'a/b')
      + expected - actual

       {
      -  "base": "a/b/.{c,"
      -  "glob": ".gitignore}"
      +  "base": "a/b"
      +  "glob": ".{c,/.gitignore}"
         "isGlob": true
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:97:45)

  5) glob-base: braces in dirname::

      AssertionError: expected { base: 'a/b/{c,.', glob: 'd}/e/f.g', isGlob: true } to equal { base: 'a/b', glob: '{c,./d}/e/f.g', isGlob: true } (at base, A has 'a/b/{c,.' and B has 'a/b')
      + expected - actual

       {
      -  "base": "a/b/{c,."
      -  "glob": "d}/e/f.g"
      +  "base": "a/b"
      +  "glob": "{c,./d}/e/f.g"
         "isGlob": true
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:105:42)



npm ERR! Test failed.  See above for more details.
npm WARN This failure might be due to the use of legacy binary "node"
npm WARN For further explanations, please read
/usr/share/doc/nodejs/README.Debian

npm ERR! not ok code 0
@jonschlinkert
Copy link
Member

ok, I'll make this change next as soon as I get a chance. A pr would also be great if someone beats me to it. thanks

@pravi
Copy link
Author

pravi commented Oct 28, 2016

I'll try to make a pr.

@es128
Copy link
Member

es128 commented Oct 28, 2016

The failures have to do with the same circumstances discussed in gulpjs/glob-parent#10 and gulpjs/glob-parent#11.

For the record, if gulpjs/glob-parent#11 were merged and published, the tests here pass.

@pravi
Copy link
Author

pravi commented Oct 28, 2016

I initially thought only tests need fixing, but I'll wait for a fix in glob-parent.

@pravi
Copy link
Author

pravi commented Oct 28, 2016

@es128 after applying gulpjs/glob-parent#11, I test still fails,

 $ npm test
> [email protected] test /home/pravi/forge/debian/git/pkg-javascript/node-glob-base
> mocha



  glob-base:
    ✓ typical globs:
    ✓ file extensions:
    1) negation pattern:
    ✓ extglobs:
    ✓ braces: no base:
    ✓ braces in filename:
    ✓ braces in dirname:
    ✓ regex character classes:
    ✓ qmarks:
    ✓ non-glob pattern:


  9 passing (49ms)
  1 failing

  1) glob-base: negation pattern::

      AssertionError: expected { base: 'a/b/c', glob: '!foo', isGlob: false } to equal { base: 'a/b/c', glob: '!foo', isGlob: true } (at isGlob, A has false and B has true)
      + expected - actual

       {
         "base": "a/b/c"
         "glob": "!foo"
      -  "isGlob": false
      +  "isGlob": true
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:76:35)



npm ERR! Test failed.  See above for more details.
npm WARN This failure might be due to the use of legacy binary "node"
npm WARN For further explanations, please read
/usr/share/doc/nodejs/README.Debian

npm ERR! not ok code 0

@es128
Copy link
Member

es128 commented Oct 28, 2016

Hmm, they all pass locally for me. Anyway, will let @jonschlinkert weigh in

@jonschlinkert
Copy link
Member

Let's revisit as soon as we get the glob-parent issue resolved. I think that will determine what we do here, and I think we're pretty close

@pravi
Copy link
Author

pravi commented Oct 28, 2016

I think, this is a bug in is-glob (3.0 as well as 3.1),

var isGlob = require('is-glob');
console.log(isGlob('a/b/c/!foo'));

gives false.

@jonschlinkert
Copy link
Member

jonschlinkert commented Oct 28, 2016

that's not a glob pattern. ! is a valid path character. To use negation inside of a path, you need to use a valid extglob, which would be a/b/c/!(foo)

edit: so this test looks like it's wrong.

@pravi
Copy link
Author

pravi commented Oct 28, 2016

then its a fix in glob-base/test.js, right?

Current test,

globBase('a/b/c/!foo').should.eql({ base: 'a/b/c', isGlob: true, glob: '!foo' });

can I change this to,

globBase('a/b/c/!foo').should.eql({ base: 'a/b/c', isGlob: false});

@jonschlinkert
Copy link
Member

then its a fix in glob-base/test.js, right?

Exactly

can I change this to,

I think that's correct, except it looks like you would also need to add glob: '' (empty string). thanks

@pravi
Copy link
Author

pravi commented Oct 28, 2016

But that seems not enough,

 {
         "base": "a/b/c"
      -  "glob": "!foo"
      +  "glob": ""
         "isGlob": false
       }

@jonschlinkert
Copy link
Member

can you clarify? Did that fail?

@pravi
Copy link
Author

pravi commented Oct 28, 2016

Yes, it gave "glob": "!foo" instead of "glob": ""

@jonschlinkert
Copy link
Member

Hmm, even with the changes from glob-parent mentioned above? I thought you were saying that the following was passing:

globBase('a/b/c/!foo').should.eql({ base: 'a/b/c', isGlob: false});

Is that correct?

@pravi
Copy link
Author

pravi commented Oct 28, 2016

I was asking if
globBase('a/b/c/!foo').should.eql({ base: 'a/b/c', isGlob: false}); would be enough. I tried your suggestion and that failed.

$ npm test

> [email protected] test /home/pravi/forge/debian/git/pkg-javascript/glob-base
> mocha



  glob-base:
    ✓ typical globs:
    ✓ file extensions:
    1) negation pattern:
    ✓ extglobs:
    ✓ braces: no base:
    ✓ braces in filename:
    ✓ braces in dirname:
    ✓ regex character classes:
    ✓ qmarks:
    ✓ non-glob pattern:


  9 passing (49ms)
  1 failing

  1) glob-base: negation pattern::

      AssertionError: expected { base: 'a/b/c', glob: '!foo', isGlob: false } to equal { base: 'a/b/c', glob: '', isGlob: false } (at glob, A has '!foo' and B has '')
      + expected - actual

       {
         "base": "a/b/c"
      -  "glob": "!foo"
      +  "glob": ""
         "isGlob": false
       }

      at Assertion.fail (node_modules/should/lib/assertion.js:196:17)
      at Assertion.prop.(anonymous function) [as eql] (node_modules/should/lib/assertion.js:81:17)
      at Context.<anonymous> (test.js:76:35)



npm ERR! Test failed.  See above for more details.
npm WARN This failure might be due to the use of legacy binary "node"
npm WARN For further explanations, please read
/usr/share/doc/nodejs/README.Debian

npm ERR! not ok code 0

@simevo
Copy link

simevo commented Dec 15, 2017

Hi in the meantime is-glob is at 4.0.0 hence PR #5 is also relevant.

What could be an estimated timeframe for fixing this ?

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

No branches or pull requests

4 participants