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

Add includeDirs to WalkOptions #601

Merged
merged 2 commits into from
Sep 18, 2019
Merged

Add includeDirs to WalkOptions #601

merged 2 commits into from
Sep 18, 2019

Conversation

nayeemrmn
Copy link
Contributor

@nayeemrmn nayeemrmn commented Sep 18, 2019

Towards API improvements in the test runner (touched upon here denoland/deno#2948 (comment)).
Also just a useful feature.

Fixes outdated documentation. cc @bartlomieju.

@bartlomieju
Copy link
Member

bartlomieju commented Sep 18, 2019

So I guess with the includeDirs option you would want to use like this: deno test src/. Then check if walk found any files/dirs, if one of found entries is a directory you'd apply default globs again. Am I understanding it correctly?

EDIT: Additionally I think that passing globs to test runner is not really ergonomic... If I do deno test **/*_test.ts then **/*_test.ts glob is expanded by my shell and test runner is passed a list of actual paths. To pass actual glob to test runner I have to write deno test \*\*\/\*_test.ts. So your solution might be more ergonomic 👍

@nayeemrmn
Copy link
Contributor Author

nayeemrmn commented Sep 18, 2019

So I guess with the includeDirs option you would want to use like this: deno test src/. Then check if walk found any files/dirs, if one of found entries is a directory you'd apply default globs again. Am I understanding it correctly?

@bartlomieju That's exactly correct! A nice consequence of this is that deno test could simply default to deno test . which would expand into deno test <DEFAULT GLOBS ROOTED AT .> by the above rule and have the same behaviour. Also, deno test src would behave the same as cd src && deno test!

I haven't thought the implementation through too deeply, but I think it would be really inefficient using any existing tools (though not in the most common usages), even with this change. There's a good chance walk wouldn't be used for it at all at some point, but this'll allow for an MVP.

@bartlomieju
Copy link
Member

I haven't thought the implementation through too deeply, but I think it would be really inefficient using any existing tools (though not in the most common usages), even with this change. There's a good chance walk wouldn't be used for it at all at some point, but this'll allow for an MVP.

It sounds fine for me, but we need to test it experimentally. Could you work on MVP?

@nayeemrmn
Copy link
Contributor Author

@ry Ready for review.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit de8d0ab into denoland:master Sep 18, 2019
@nayeemrmn nayeemrmn deleted the fs-walk branch September 18, 2019 16:58
@nayeemrmn nayeemrmn mentioned this pull request Sep 18, 2019
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
inverted-capital pushed a commit to dreamcatcher-tech/napps that referenced this pull request Oct 24, 2024
This change adds a "Go to my profile" link and changes the styling and
positions of links on the account page.
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.

3 participants