Implicitly consider an extensionless file in "includes" to be a recursive directory glob #11495
Conversation
ghost
assigned …sive directory glob
ghost
|
I tested this on the command-line with a real project and it worked. I'd still like to get @rbuckton's review before merging. Are there some additional tests that should be added? |
| @@ -1289,6 +1287,18 @@ namespace ts { | |||
| return wildcardDirectories; | |||
| } | |||
|
|
|||
| function getWildcardDirectoryFromSpec(spec: string, useCaseSensitiveFileNames: boolean): { key: string, flags: WatchDirectoryFlags } | undefined { | |||
| const match = wildcardDirectoryPattern.exec(spec); | |||
| return match | |||
There was a problem hiding this comment.
can you turn this into a series of ifs? this is pretty unreadable to me.
| } | ||
| } | ||
| /** | ||
| * An "includes" path "foo" is implicitly a glob "foo\**\*" (replace \ with /) if its last component has no extension, |
There was a problem hiding this comment.
what do you mean by "replace \ with /"?
| @@ -1186,6 +1186,12 @@ namespace ts { | |||
| return path.substr(0, Math.max(getRootLength(path), path.lastIndexOf(directorySeparator))); | |||
| } | |||
|
|
|||
| function getBasename(path: Path): Path; | |||
| function getBasename(path: string): string; | |||
| function getBasename(path: string): any { | |||
There was a problem hiding this comment.
You shouldn't need to use any - our overload compatibility rules are bidirectional for return types.
| else if (isImplicitGlob(spec)) { | ||
| return { key: spec, flags: WatchDirectoryFlags.Recursive }; | ||
| } | ||
| else { |
There was a problem hiding this comment.
Nit: when each branch of an if finishes in a return, we don't structure these as if-elses, since the logic is automatically exclusive.
| if (hasWrittenComponent) { | ||
| subpattern += directorySeparator; | ||
| } | ||
| return "^(" + pattern + (usage === "exclude" ? ")($|/)" : ")$"); |
There was a problem hiding this comment.
Separate the last part into a variable and comment what the purpose is.
| } | ||
| else { | ||
| return absolute.substring(0, absolute.lastIndexOf(directorySeparator, wildcardOffset)); | ||
| } |
There was a problem hiding this comment.
Same nit as above about if-elses
| function getIncludeBasePath(absolute: string): string { | ||
| const wildcardOffset = indexOfAnyCharCode(absolute, wildcardCharCodes); | ||
| if (wildcardOffset < 0) { | ||
| // No "*" in the path |
There was a problem hiding this comment.
Isn't isImplicitGlob redoing the work of the call to indexOfAnyCharCode above? We have already eliminated * and ? as possible characters, so the only character you really need to test for is ..
Also the comment should probably read // No "*" or "?" in the path
Fixes #10578
There is a lot of refactoring in this PR but the actual changes just have to do with uses of the
isImpicitGlobfunction. If this always returnsfalsethen behavior will be the same as before. If wanted I could break this into 2 PRs, one which refactors and one which addsisImplicitGlob.It would also be a good idea to someday rewrite this functionality into a) parsing globs, then b) doing things with parsed globs. Then we would pass around strings less often.