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

Glob integration for the FS walker #219

Merged
merged 16 commits into from Mar 2, 2019

Conversation

4 participants
@zekth
Copy link
Contributor

zekth commented Feb 28, 2019

Reference: denoland/deno#1856

This is the implementation discussed with @hayd . There is still a problem with the matches of the walker on certain cases.

Working on it.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Feb 28, 2019

CLA assistant check
All committers have signed the CLA.

zekth added some commits Feb 28, 2019

fix
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Mar 1, 2019

I think you need to patch walk.ts to remove the ./ in the match:

function include(f: FileInfo, options: WalkOptions): Boolean {
  const path = f.path.startsWith("./") ? f.path.slice(2) : f.path;
  if (options.exts && !options.exts.some(ext => path.endsWith(ext))) {
    return false;
  }
  if (options.match && !options.match.some(pattern => pattern.test(path))) {
    return false;
  }
  if (options.skip && options.skip.some(pattern => pattern.test(path))) {
    return false;
  }
  return true;
}

Tests still don't pass, but that gets you closer...

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Mar 1, 2019

Actually, that shouldn't be required. Trying to debug, something strange seems to be going on!

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Mar 1, 2019

It seems like there's an issue here:

  if (options.match && !options.match.some(pattern => pattern.test(path))) {
    return false;
  }

that although options.match && !options.match.some(pattern => pattern.test(path)) is false, it still ends up inside the if statement. Which makes no sense!


testWalk(
async (d: string) => {
await touch(d + "x.ts");

This comment has been minimized.

@hayd

hayd Mar 1, 2019

Contributor

these should be touch(d + "/x.ts")

This, and other glob tests, may not work on windows because of this...

This comment has been minimized.

@zekth

zekth Mar 1, 2019

Author Contributor

you're right. Changed it but now the assertion is wrong. The walk returns only 1 item which is:
./x.ts and normally it would return ./x.js also.

This comment has been minimized.

@hayd

hayd Mar 1, 2019

Contributor

Something is definitely weird. I think it's in that include function, in the if statement, but was unable to nail it down / work out what is causing it.

This comment has been minimized.

@hayd

hayd Mar 1, 2019

Contributor

Is the pattern somehow mutable? I get these being different!!

  const a = options.match && (!options.match.some(pattern => pattern.test(path)));
  const b = options.match && (!options.match.some(pattern => pattern.test(path)));
  const c = options.match && (!options.match.some(pattern => pattern.test(path)));
  console.log(a, b, c)

This comment has been minimized.

@zekth

zekth Mar 1, 2019

Author Contributor

This is due to the g flag.
See: https://stackoverflow.com/questions/1520800/why-does-a-regexp-with-global-flag-give-wrong-results

So in this case we might have to change this part in globrex:

    if (!flags.includes('g')) {
        regex = `^${regex}$`;
        segment = `^${segment}$`;
        if (filepath) path.regex = `^${path.regex}$`;
    }

To only get the generated regex pattern. Like returning the raw string expression and cast it by ourselves in the glob function.

This comment has been minimized.

@hayd

hayd Mar 1, 2019

Contributor

This is like Hitchhiker's Guide to the Galaxy API design here. "That pitfall that you fell in has been perfectly documented in the spec for several years, if you had only bothered to check"

very cool!

Not sure whether that's best way to fix, or if should use the lastIndex reset after each use.

This comment has been minimized.

@zekth

zekth Mar 1, 2019

Author Contributor

https://github.com/denoland/deno_std/pull/219/files#diff-f7a9ec5a7cf0d6e9d8822f7ab61bf494R100

added it and supposed to work. Trying to figure out with the assertion is wrong.

for example this script:

var arr = ["x.js", "x.ts", "z.js"];
console.log('array test without')
let arrReg = [RegExp(/^x\..*$/, "g")]
arr.forEach(path => {
  let r = arrReg.some(pattern => {
    var f = pattern.test(path);
    return f;
  });
  console.log(`${path}:${r}`);
});

console.log('array test with')
let arrReg2 = [RegExp(/^x\..*$/, "g")]
arr.forEach(path => {
  let r = arrReg2.some(pattern => {
    var f = pattern.test(path);
    pattern.lastIndex = 0;
    return f;
  });
  console.log(`${path}:${r}`);
});

outputs

array test without
x.js:true
x.ts:false
z.js:false
array test with
x.js:true
x.ts:true
z.js:false
Show resolved Hide resolved fs/glob.ts Outdated

zekth added some commits Mar 1, 2019

Show resolved Hide resolved fs/walk.ts Outdated
Show resolved Hide resolved fs/globrex.ts

@zekth zekth changed the title WIP: Glob integration for the FS walker Glob integration for the FS walker Mar 2, 2019

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Mar 2, 2019

lgtm

FYI @ry this is direct copy of https://github.com/hayd/deno-globrex (which is a port of globrex, with very minor changes), with a small wrapper in glob.ts... and a pretty wild RegExp fix to walk.

Once we have this in std we can use globs in --fmt and --test (and other cli apps).

@zekth

This comment has been minimized.

Copy link
Contributor Author

zekth commented Mar 2, 2019

By the way i thought about the exposition of GlobOptions interface. At the moment it's in globrex module. Would it be better to expose it in glob module? Because globrex is not really meant to be exposed publicly? Right?

import { FileInfo } from "deno";
import { globrex, GlobOptions } from "./globrex.ts";

export function glob(glob: string, options: GlobOptions = {}): RegExp {

This comment has been minimized.

@ry

ry Mar 2, 2019

Contributor

In future commits please add jsdocs to exported methods. (With code examples.)

This comment has been minimized.

@zekth

zekth Mar 2, 2019

Author Contributor

For sure i was thinking about it. Have to check with @hayd what's the best place and examples for glob usage. Working on this asap!

@ry

ry approved these changes Mar 2, 2019

Copy link
Contributor

ry left a comment

LGTM

@ry ry merged commit 0c3ba83 into denoland:master Mar 2, 2019

2 checks passed

denoland.deno_std #20190302.1 succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@zekth zekth deleted the zekth:glob branch Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.