-
Notifications
You must be signed in to change notification settings - Fork 766
General correction of glob patterns and config file locating #742
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
Changes from 8 commits
ed379ea
668d104
cd7970d
3c80f10
ac777fc
7728227
4227851
4b9ca6c
a6bb62d
b9d45ef
28c5e22
f98996b
a234012
bc3b235
2a6557f
91bc957
2cf1f5f
34b7a4c
e5ed318
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,21 +40,32 @@ export class TSConfigReader extends OptionsComponent { | |
| return; | ||
| } | ||
|
|
||
| let file: string; | ||
|
|
||
| if (TSConfigReader.OPTIONS_KEY in event.data) { | ||
| this.load(event, Path.resolve(event.data[TSConfigReader.OPTIONS_KEY])); | ||
| const tsconfig = event.data[TSConfigReader.OPTIONS_KEY]; | ||
|
|
||
| if (/tsconfig\.json$/.test(tsconfig)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this line is responsible for making typedoc 0.14 a breaking change for my projects. The path that I use for my config file is something like This worked fine in 0.13 but breaks in 0.14. I now need to update the path on 60+ projects.. |
||
| file = Path.resolve(tsconfig); | ||
| } else { | ||
| file = ts.findConfigFile(tsconfig, ts.sys.fileExists); | ||
| } | ||
|
|
||
| if (!file || !FS.existsSync(file)) { | ||
| event.addError('The tsconfig file %s does not exist.', file); | ||
| return; | ||
| } | ||
| } else if (TSConfigReader.PROJECT_KEY in event.data) { | ||
| // The `project` option may be a directory or file, so use TS to find it | ||
| let file: string = ts.findConfigFile(event.data[TSConfigReader.PROJECT_KEY], ts.sys.fileExists); | ||
| // If file is undefined, we found no file to load. | ||
| if (file) { | ||
| this.load(event, file); | ||
| } | ||
| file = ts.findConfigFile(event.data[TSConfigReader.PROJECT_KEY], ts.sys.fileExists); | ||
| } else if (this.application.isCLI) { | ||
| let file: string = ts.findConfigFile('.', ts.sys.fileExists); | ||
| // If file is undefined, we found no file to load. | ||
| if (file) { | ||
| this.load(event, file); | ||
| } | ||
| // No file or directory has been specified so find the file in the root of the project | ||
| file = ts.findConfigFile('.', ts.sys.fileExists); | ||
| } | ||
|
|
||
| // If file is undefined, we found no file to load. | ||
| if (file) { | ||
| this.load(event, file); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -65,14 +76,9 @@ export class TSConfigReader extends OptionsComponent { | |
| * @param fileName The absolute path and file name of the tsconfig file. | ||
| */ | ||
| load(event: DiscoverEvent, fileName: string) { | ||
| if (!FS.existsSync(fileName)) { | ||
| event.addError('The tsconfig file %s does not exist.', fileName); | ||
| return; | ||
| } | ||
|
|
||
| const { config } = ts.readConfigFile(fileName, ts.sys.readFile); | ||
| if (config === undefined) { | ||
| event.addError('The tsconfig file %s does not contain valid JSON.', fileName); | ||
| event.addError('No valid tsconfig file found.', fileName); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the text since it now is used when there is no file as well as an invalid file |
||
| return; | ||
| } | ||
| if (!_.isPlainObject(config)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| import * as Path from 'path'; | ||
| import { isArray } from 'util'; | ||
| import { Minimatch, IMinimatch } from 'minimatch'; | ||
|
|
||
| function createMinimatch(pattern: string): IMinimatch { | ||
| if (pattern[0] === '.' || pattern[0] === '/' || /^\w+(?!:)($|[/\\])/.test(pattern)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain this regex? I don't have the skills to understand it. Is there any of this that Minimatch handles already?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok so this
Which are all relative paths and should be resolved relative to The regex is to test that windows absolute paths like I could have used one regex to test for all of these cases, but it is a lot faster to do the simple tests first and only do the regex if really needed. The main issue is that generally in other libraries that uses GLOB expressions you can just type
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But I can see I made a mistake: |
||
| pattern = Path.resolve(pattern); | ||
| } | ||
|
|
||
| return new Minimatch(pattern.replace(/[\\]/g, '/'), { dot: true }); | ||
| } | ||
|
|
||
| export function pathToMinimatch(pattern: string | string[]): IMinimatch | IMinimatch[] { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this function really needed? From the usage I saw, it seems that I'd prefer to inline the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is true that the two methods could be combined. I separated them in an attempt to make the code less bulky and easier to read. But if you prefer to combine them I can. It is also a bit slower to recreate a function inside a function, so there is also a minor performance gain of having them separate. EDIT
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. regarding the |
||
| return isArray(pattern) | ||
| ? pattern.map(createMinimatch) | ||
| : createMinimatch(pattern); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next time, avoid updating dependencies unnecessarially. It creates merge conflicts with other contributions.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok got it... I will revert it