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

Update of glob-style pattern matching #8841

Merged
merged 26 commits into from
Jun 20, 2016
Merged

Update of glob-style pattern matching #8841

merged 26 commits into from
Jun 20, 2016

Conversation

riknoll
Copy link
Member

@riknoll riknoll commented May 26, 2016

This is a resubmission of #5980 that has been brought up to date with master (the old PR had a lot of non-trivial merge conflicts with the current master). The behavior differs somewhat from the specification in #5980 to better reflect the current behavior of the compiler. Excluded patterns now default to the common package directories (node_modules, bower_components, etc.) when "exclude" is not found in tsconfig.json. Additionally, the out directory is always excluded unless explicitly included in "files".

@mhegazy @rbuckton @billti please take a look when you have a moment!

@msftclas
Copy link

Hi @riknoll, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Richard Knoll). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@riknoll
Copy link
Member Author

riknoll commented Jun 17, 2016

@rbuckton could you take a look when you have a moment?

continue;
}

if (IgnoreFileNamePattern.test(file)) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: capitalization - ignoreFileNamePattern

@riknoll
Copy link
Member Author

riknoll commented Jun 20, 2016

@rbuckton thanks!

@riknoll riknoll merged commit 6d99b2f into master Jun 20, 2016
@basarat
Copy link
Contributor

basarat commented Jun 20, 2016

wow 🎉 🌹

@mhegazy mhegazy deleted the glob2_merged branch June 20, 2016 23:25
@mhegazy
Copy link
Contributor

mhegazy commented Jun 20, 2016

//CC: @basarat, @jrieken, @blakeembrey @jbrantly, @adidahiya, @alexeagle, @martine

this change has a breaking change to the public API, changing the arguments to ParseConfigHost.readDirectory and System.readDirectory, from

readDirectory(rootDir: string, extension: string, exclude: string[]): string[];

to:

readDirectory(rootDir: string, extensions: string[], excludes: string[], includes: string[]): string[];

Please do let us know if this is something that manageable on your side or more changes needs to be done on the TS side.

@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Jun 20, 2016
@mhegazy mhegazy modified the milestones: Community, TypeScript 2.0 Jun 20, 2016
@mhegazy mhegazy added the API Relates to the public API for TypeScript label Jun 20, 2016
@johnnyreilly
Copy link

Awesome!😃

@evmar
Copy link
Contributor

evmar commented Jun 22, 2016

@mhegazy thanks for thinking of me! I didn't see any use/impl of readDirectory in Google code.

In Angular2 I found one use @alexeagle , but it's likely easy to update:
https://github.com/angular/angular/blob/3d5bb2318454ba428b816f6aee6e6aaf427d70a7/tools/%40angular/tsc-wrapped/src/tsc.ts#L51

In general I think we (Google) don't mind too much when you change APIs, especially when it's a small change that the compiler can easily spot when we update. And usually the only reason to change an API in a large way is because it's important so we'll likely be able to make that work as well.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 22, 2016

thanks @martine for getting to us back on this.

alexeagle added a commit to alexeagle/angular that referenced this pull request Jun 23, 2016
This is required due to breaking change in TS, see
microsoft/TypeScript#8841 (comment)
alexeagle added a commit to alexeagle/angular that referenced this pull request Jun 23, 2016
This is required due to breaking change in TS, see
microsoft/TypeScript#8841 (comment)
alexeagle added a commit to alexeagle/angular that referenced this pull request Jun 23, 2016
This is required due to breaking change in TS, see
microsoft/TypeScript#8841 (comment)
alexeagle added a commit to angular/angular that referenced this pull request Jun 24, 2016
This is required due to breaking change in TS, see
microsoft/TypeScript#8841 (comment)
@texastoland
Copy link
Contributor

I'm having trouble excluding arbitrarily deep node_modules. Should **/node_modules or **/node_modules/**/* do the trick?

@basarat
Copy link
Contributor

basarat commented Jul 7, 2016

I'm having trouble excluding arbitrarily deep node_modules.

they might still get pulled in because of import statements 🌹

@riknoll
Copy link
Member Author

riknoll commented Jul 7, 2016

**/node_modules should work, but @basarat's comment about imports is also correct

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Relates to the public API for TypeScript Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants