General correction of glob patterns and config file locating #742

Tokimon · 2018-04-06T10:27:54Z

This PR main purpose is to optimize the options that uses glob patterns, as they were not working optimally, as well as making sure config files are found correctly.

(unit test added for the general Minimatch function)

Glob pattern options
--externalPattern and --exclude options now behaves according to expectations of many developers.

Relative paths:
./my/path/**, ../my/path/** and my/path/** all correctly matches paths relative to the project root.
Absolute paths:
/unix-style/path/**, C:/win-style/path/** and **/my/path/** all still matches full paths as before.
{ dot: true } has been added to the Minimatch as per PR Allow exclusion of directories starting with a dot #711

typedoc.js(on)

If no typedoc file is indicated with the --options option any typedoc.js or typedoc.json will automatically be searched for in the root of the project (with typedoc.js taking precedence).
If a directory is defined in the --options option any typedoc.js or typedoc.json in the root of that directory will be searched for (with typedoc.js taking precedence). If no file is found in the directory an error is raised.
Specifying the exact file is still viable and result in an error if not found

tsconfig.json
Original functionality has not been altered, but --tsconfig can now also take a directory path instead of only a direct tsconfig.json path.

- Also allowed it to take multiple path expressions

aciccarello · 2018-04-06T14:55:36Z

Looks great! Thanks for the contribution. I had started working on getting the typedoc.json file working again a while ago but never finished.

I'll take a little more time to look this over before merging but from what I see, it looks great!

aciccarello

Love the cleanup of file loading. I don't fully understand the changes to where minimatch is being used?

Also, the files have a 4 space indent style already so new code should probably stick with that. There should be some tooling to apply that already.

aciccarello · 2018-04-12T04:04:01Z

package.json

@@ -33,34 +33,34 @@
    "@types/fs-extra": "5.0.1",
    "@types/handlebars": "4.0.36",
    "@types/highlight.js": "9.12.2",
-    "@types/lodash": "4.14.104",
+    "@types/lodash": "4.14.106",


Next time, avoid updating dependencies unnecessarially. It creates merge conflicts with other contributions.

ok got it... I will revert it

aciccarello · 2018-04-12T04:05:58Z

src/lib/utils/options/readers/tsconfig.ts

        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);


I changed the text since it now is used when there is no file as well as an invalid file

aciccarello · 2018-04-12T04:07:52Z

src/lib/utils/paths.ts

+  return new Minimatch(pattern.replace(/[\\]/g, '/'), { dot: true });
+}
+
+export function pathToMinimatch(pattern: string | string[]): IMinimatch | IMinimatch[] {


Is this function really needed? From the usage I saw, it seems that createMinimatch would be sufficient. The overloading of a parameter that is sometimes an array means that you frequently need to cast the result.

I'd prefer to inline the arr.map(createMinimatch) in files.

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
So reading my code again I Remember why I made it as a separate function. It is to keep the given pattern argument as a string or Array which ever format it has. So I use the same method in two different scenarios.
But if you want I can do a method override to make it clearer.

regarding the pattern: string | string[] is because the given path can be one or several paths (as parsed from the CLI arguments). When only one path is given it is just a string otherwise an array of strings is given, hence the double type definition.

aciccarello · 2018-04-12T04:08:53Z

src/lib/utils/paths.ts

+import { Minimatch, IMinimatch } from 'minimatch';
+
+function createMinimatch(pattern: string): IMinimatch {
+  if (pattern[0] === '.' || pattern[0] === '/' || /^\w+(?!:)($|[/\\])/.test(pattern)) {


Can you explain this regex? I don't have the skills to understand it.

Is there any of this that Minimatch handles already?

Ok so this if is to check if a path like these:

./some/path

../some/path

some/path

path

Which are all relative paths and should be resolved relative to process.cwd() (Path.resolve(path)).

The regex is to test that windows absolute paths like C:/some/path wont be confused for a relative path like c/some/path. So I use a negative look ahead to ensure there is no : after the letters in the first part of the path (path/ will be a relative path and path:/ won't)

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 ./some/path and that would be treated as relative to the project root, but this have not been the case for this project where **/some/path was needed to ensure correct path matching, which has been counter-intuitive for many developers.

But I can see I made a mistake: pattern[0] === '/' would match /some/path as relative, which isn't the case. I will fix this.

aciccarello · 2018-04-12T04:09:17Z

src/test/utils.paths.ts

+// Used to ensure uniform path cross OS
+const absolutePath = (path: string) => Path.resolve(path).replace(/[\\]/g, '/');
+
+describe('Paths', () => {


Thanks for writing tests!

Tokimon · 2018-04-13T10:33:20Z

I created the general function for the minimatch to uniform the the way the minimatching is done (right now it is used in two places), so the change is basically to allow paths to be written as relative to the project root (process.cwd()) as is the norm in many other modules and it have confused several developers which have asked how to do the exclude for example only to find that they needed ** in front of the path to make it work (my self included).

I will check up on the indentation as well.

This reverts commit 7728227.

- Renamed from `pathToMinimatch` - Added overload methods for easier usage (no need for explicit casting) and clarification - Updated how absolute and relative paths are checked - Fixed indentation

aciccarello

Looks good! The createMinimatch code is a lot easier to understand. Still a couple questions

aciccarello · 2018-04-13T16:20:48Z

src/lib/utils/paths.ts

+    // Ensure uniform absolute path cross OS
+    // (POSIX would resolve c:/path to /path/to/repo/c:/path without this check)
+    if (Path.isAbsolute(pattern) && Path.sep === '/') {
+        pattern = pattern.replace(/^\w:/, '');


If the drive letter is removed, does this break access to other drives (e.g. d:/path) or was that already broken?

No it shouldn't.
At least to my knowledge. As far as I understand it the drive letter notation is really a windows only thing and on other systems rather use names for different drives like /externaldrive/some/path or /drive2/some/path.
Therefore any drive notation is stripped on these systems, but mostly as a precaution, as you never know how people write their paths.
However drive notated paths on Windows systems are kept intact (thanks to the Path.sep === '/' check which ensures this is only done on non Windows machines).

You could argue that the check is redundant, as people probably wouldn't use the drive notation anyway. But I am just so used to try and foresee stupid errors that I added it.

Let me know if you think we keep it or not.

Okay, thanks for explaining. We can leave it.

Just an update on the UNIX pathing system. I asked around and for UNIX systems (Linxus + MAC) you change drive by typing something like /volumes/drive. So absolute paths always start with a '/' on those systems. So it should be good like this.

aciccarello · 2018-04-13T16:35:40Z

src/lib/utils/paths.ts

+
+export function createMinimatch(pattern: string[]): IMinimatch[];
+export function createMinimatch(pattern: string): IMinimatch;
+export function createMinimatch(pattern: string | string[]): IMinimatch | IMinimatch[] {


since all usages are IMinimatch[] and to avoid overloaded functions, please remove this function and inline x.map(convertToMinimatch) where createMinimatch was being used.

ok... if you prefer, I can remove the overloads, but as the function is used two places already (which is why I created it in the first place), so I prefer to keep the function it self to avoid having duplicate the code in the code base.
And it is easier to unit test the method used if it is isolated like this.

okay, that sounds good to me

aciccarello · 2018-04-13T17:10:26Z

src/lib/utils/paths.ts

+        pattern = pattern.replace(/^\w:/, '');
+    }
+
+    if (pattern[0] !== '*') {


Should this be checking for **? If the path is not resolved for */test.js then I think it won't work with /project/base/*/test.js which is what I would expect

hm... yeah you might be right... I will correct it.

…re relative

aciccarello

Great! Thanks for iterating on this with me. I'll merge after I get some issues with tests on master fixed.

Tokimon · 2018-04-19T14:55:33Z

Sure thing! Thanks for your input.
I will try and get the Linux test to work as well though as it seems that Linus is not parsing the paths as expected 😢

Tokimon · 2018-04-25T21:57:42Z

There we go. Tests are passing now.

Strict null checks required a few changes to code changed by this PR.

Gerrit0 · 2018-10-28T16:04:16Z

Thanks for the contribution! I'm sorry this took so long to merge.

Roaders · 2019-01-10T06:19:08Z

src/lib/utils/options/readers/tsconfig.ts

-            this.load(event, Path.resolve(event.data[TSConfigReader.OPTIONS_KEY]));
+            const tsconfig = event.data[TSConfigReader.OPTIONS_KEY];
+
+            if (/tsconfig\.json$/.test(tsconfig)) {


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 node_modules/my-config-project/config/typedoc.config.json

This worked fine in 0.13 but breaks in 0.14.

I now need to update the path on 60+ projects..

Toke Voltelen added 7 commits April 6, 2018 11:56

Add paths util

ed379ea

Add general minimatch function to externalPattern

668d104

- Also allowed it to take multiple path expressions

Add general minimatch function to exclude

cd7970d

Fix typedoc.js(on) options file finding

3c80f10

Optimize tsconfig.json file finding

ac777fc

Update dependencies

7728227

Add unit tests for matching paths

4227851

aciccarello self-requested a review April 6, 2018 14:55

aciccarello added 2 commits April 11, 2018 21:45

style tweaks

4b9ca6c

add file name back into tsconfig load error

a6bb62d

aciccarello requested changes Apr 12, 2018

View reviewed changes

Toke Voltelen added 4 commits April 13, 2018 13:56

Revert "Update dependencies"

b9d45ef

This reverts commit 7728227.

Force 4 spaces as indent for the TS Lint

28c5e22

createMinimatch fixed

f98996b

- Renamed from `pathToMinimatch` - Added overload methods for easier usage (no need for explicit casting) and clarification - Updated how absolute and relative paths are checked - Fixed indentation

updated @types/mocha to match current mocha version

a234012

aciccarello requested changes Apr 13, 2018

View reviewed changes

aciccarello and others added 3 commits April 13, 2018 11:54

Merge branch 'master' into fix/paths

bc3b235

CreateMinimatch always assumes array of strings + single star paths a…

2a6557f

…re relative

Correct createMinimatch unit test

91bc957

aciccarello approved these changes Apr 18, 2018

View reviewed changes

Correct posix pathing

2cf1f5f

Tokimon mentioned this pull request Apr 30, 2018

typedoc.js Stops Working after Writing Configuration in tsconfig.js is Supported. #604

Closed

pnbecker mentioned this pull request May 11, 2018

yarn run docs has problems finding the options DSpace/dspace-angular#265

Open

Gerrit0 added 2 commits October 28, 2018 08:18

Merge branch 'master' into pr/742

34b7a4c

Resolve build errors

e5ed318

Strict null checks required a few changes to code changed by this PR.

Gerrit0 merged commit dbdbfca into TypeStrong:master Oct 28, 2018

Roaders reviewed Jan 10, 2019

View reviewed changes

dokmic mentioned this pull request Apr 25, 2019

Negated/commented Minimatch expressions are not working #1024

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General correction of glob patterns and config file locating #742

General correction of glob patterns and config file locating #742

Tokimon commented Apr 6, 2018

aciccarello commented Apr 6, 2018

aciccarello left a comment

aciccarello Apr 12, 2018

Tokimon Apr 13, 2018 •

edited

aciccarello Apr 12, 2018

aciccarello Apr 12, 2018

Tokimon Apr 13, 2018 •

edited

Tokimon Apr 13, 2018 •

edited

aciccarello Apr 12, 2018

Tokimon Apr 13, 2018 •

edited

Tokimon Apr 13, 2018

aciccarello Apr 12, 2018

Tokimon Apr 13, 2018

Tokimon commented Apr 13, 2018

aciccarello left a comment

aciccarello Apr 13, 2018

Tokimon Apr 15, 2018 •

edited

aciccarello Apr 17, 2018

Tokimon Apr 17, 2018

aciccarello Apr 13, 2018

Tokimon Apr 15, 2018 •

edited

aciccarello Apr 17, 2018

aciccarello Apr 13, 2018

Tokimon Apr 15, 2018

aciccarello left a comment

Tokimon commented Apr 19, 2018 •

edited

Tokimon commented Apr 25, 2018

Gerrit0 commented Oct 28, 2018

Roaders Jan 10, 2019

General correction of glob patterns and config file locating #742

General correction of glob patterns and config file locating #742

Conversation

Tokimon commented Apr 6, 2018

aciccarello commented Apr 6, 2018

aciccarello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokimon Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokimon Apr 13, 2018 • edited

Choose a reason for hiding this comment

Tokimon Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokimon Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokimon commented Apr 13, 2018

aciccarello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokimon Apr 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tokimon Apr 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aciccarello left a comment

Choose a reason for hiding this comment

Tokimon commented Apr 19, 2018 • edited

Tokimon commented Apr 25, 2018

Gerrit0 commented Oct 28, 2018

Choose a reason for hiding this comment

Tokimon Apr 13, 2018 •

edited

Tokimon Apr 13, 2018 •

edited

Tokimon Apr 13, 2018 •

edited

Tokimon Apr 13, 2018 •

edited

Tokimon Apr 15, 2018 •

edited

Tokimon Apr 15, 2018 •

edited

Tokimon commented Apr 19, 2018 •

edited