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

Implement expandGlob() #617

Merged
merged 20 commits into from Oct 2, 2019

Conversation

@nayeemrmn
Copy link
Contributor

commented Sep 28, 2019

...properly. Fixes #576.

This had to be done from scratch unless we could utilise globrex somehow but I think it can be done by splitting paths and flat-mapping on each segment, applying globrex on individual segments. I also want to give it an exclude option so excluded directories won't be traversed - callers all want to exclude by path prefix anyway, this is currently done with a hacky g flag.

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from 146b76a to f4cc885 Sep 28, 2019
nayeemrmn added 2 commits Sep 28, 2019
Regression. The prettier tests will fail until expandGlob() is fixed
since they try to match absolute paths outside of CWD.
Should help later when extracting common functionality to expandGlob(),
like exclusion. Makes findTestModules() an async generator.
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from f4cc885 to 4729107 Sep 28, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from a2e441f to b6af0a7 Sep 28, 2019
Adds an exclude field to ExpandGlopOptions. This should be the final
API.
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from f1d05be to de35f8f Sep 29, 2019
In other words, support path prefixes when specifying excludes. This
aligns with the file matchers of the test runner and prettier. It's less
redundant since this cannot be achieved efficiently on the client-end.
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from de35f8f to eda4eb3 Sep 29, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch 5 times, most recently from 437e930 to 9b1b634 Sep 29, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from 9b1b634 to a783ffc Sep 29, 2019
@nayeemrmn nayeemrmn changed the title Implement expandGlob() [WIP] Implement expandGlob() Sep 29, 2019
@nayeemrmn nayeemrmn marked this pull request as ready for review Sep 29, 2019
@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2019

@ry @bartlomieju Ready for review!

I've checked that expandGlobSync() on any direct path with an absolute root provided uses just one op! This affects the test runner and prettier. File-system walking is more or less minimised.

fs/walk.ts Show resolved Hide resolved
@ry

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

@kt3k a review from you would be appreciated if you have the time.

fs/glob.ts Outdated Show resolved Hide resolved
fs/glob.ts Show resolved Hide resolved
fs/glob.ts Outdated Show resolved Hide resolved
fs/glob.ts Show resolved Hide resolved
@ry

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2019

@nayeemrmn In your description, it says "DON'T MERGE". Please update if it is no longer applicable.

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from 3c09cc8 to ceca937 Oct 1, 2019
fs/glob.ts Show resolved Hide resolved
@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

@ry Fixed! Ready again.

@kt3k

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

//fs/README has glob entry. I think we need to update that to globToRegExp as well.

fs/glob.ts Outdated Show resolved Hide resolved
fs/walk.ts Outdated Show resolved Hide resolved
fs/walk_test.ts Show resolved Hide resolved
@kt3k
kt3k approved these changes Oct 2, 2019
Copy link
Contributor

left a comment

LGTM

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from 64fad83 to 87f51c8 Oct 2, 2019
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from 87f51c8 to 3b1b189 Oct 2, 2019
@ry
ry approved these changes Oct 2, 2019
Copy link
Contributor

left a comment

LGTM - nice PR @nayeemrmn!

(And thanks for the careful review @kt3k )

@ry

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

@nayeemrmn Can you draft a commit message so I can squash this all up?

fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from #604.
- Rename glob() to globToRegExp().
- Add normalizeGlob() and joinGlobs().
- Extract GlobToRegExpOptions from GlobOptions, remove the strict
  and filepath options.

fs/globrex.ts:
- Add GlobrexOptions.

fs/path/constants.ts:
- Add SEP_PATTERN.

fs/walk.ts:
- Add WalkOptions::includeFiles
- Default WalkOptions::includeDirs to true.
- Don't traverse directories matching a skip pattern.
- Remove walkSync()'s default root value.

prettier:
- Refactor to use expandGlob().

testing:
- Make findTestModules() an async generator.
@nayeemrmn nayeemrmn force-pushed the nayeemrmn:expand-glob branch from c8c04b4 to eb666fd Oct 2, 2019
@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

@ry Added an empty commit with the message.

@ry ry merged commit 8c90bd9 into denoland:master Oct 2, 2019
5 checks passed
5 checks passed
denoland.deno_std Build #20191002.7 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@nayeemrmn nayeemrmn deleted the nayeemrmn:expand-glob branch Oct 2, 2019
ry added a commit to ry/deno that referenced this pull request Oct 9, 2019
fs/glob.ts:
- Improve prototypes for expandGlob() and expandGlobSync() from denoland/deno_std#604.
- Rename glob() to globToRegExp().
- Add normalizeGlob() and joinGlobs().
- Extract GlobToRegExpOptions from GlobOptions, remove the strict
  and filepath options.

fs/globrex.ts:
- Add GlobrexOptions.

fs/path/constants.ts:
- Add SEP_PATTERN.

fs/walk.ts:
- Add WalkOptions::includeFiles
- Default WalkOptions::includeDirs to true.
- Don't traverse directories matching a skip pattern.
- Remove walkSync()'s default root value.

prettier:
- Refactor to use expandGlob().

testing:
- Make findTestModules() an async generator.
Original: denoland/deno_std@8c90bd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.