Properly handle include, exclude globs (#675)

## Summary:
This fixes a bug where we treated input globs as ignore format globs.
This speeds up our processing as now file paths provided at the command-line
won't lead to extra glob parsing.

Also refactors a few things for easier following of the code and test isolation.
In addition, the main entrypoint is now `main.js`. This paves the way for us
to also export an API. Some more work to be done there though as the `checkSync`
function is not sufficiently isolated for that to be the API.

Issue: Fixes #671

## Test plan:
`yarn test`

Author: somewhatabstract

Reviewers: somewhatabstract, kevinbarabash

Required Reviewers: 

Approved by: 

Checks: ⌛ Lint and flow check (ubuntu-latest, 16.x), ⌛ Update test coverage (ubuntu-latest, 16.x), ⌛ Analyze (javascript), ⏭  dependabot

Pull request URL: #675

bin/checksync.dev.js

@@ -1,3 +1,3 @@
 #!/usr/bin/env node
 require("@babel/register");
-require("../src/cli.js").run(__filename);
+require("../src/main.js").run(__filename);

bin/checksync.js

@@ -1,2 +1,2 @@
 #!/usr/bin/env node
-require("../dist/cli.js").run(__filename);
+require("../dist/main.js").run(__filename);

package.json

@@ -2,7 +2,7 @@
     "name": "checksync",
     "version": "2.3.0",
     "description": "A tool that allows code to be annotated across different files to ensure they remain in sync.",
-    "main": "dist/cli.js",
+    "main": "dist/main.js",
     "bin": {
         "checksync": "./bin/checksync.js"
     },

rollup.config.js

@@ -26,9 +26,9 @@ const getOptionalPlugins = () => {
 };
 
 export default {
-    input: "./src/cli.js",
+    input: "./src/main.js",
     output: {
-        file: "./dist/cli.js",
+        file: "./dist/main.js",
         format: "cjs",
     },
     plugins: [

src/__tests__/__snapshots__/cli_test.js.snap

@@ -47,15 +47,15 @@ Where:
 
 Arguments
 
-    [1m[33m--comments,-c[39m[22m      A string containing comma-separated tokens that
+    [1m[33m--comments,-c[39m[22m      A string containing space-separated tokens that
                        indicate the start of lines where tags appear.
-                       Defaults to [1m[33m\\"//,#\\"[39m[22m.
+                       Defaults to [1m[33m\\"// #\\"[39m[22m.
 
     --dry-run,-n       Ignored unless supplied with --update-tags.
 
     --help,-h          Outputs this help text.
 
-    [1m[33m--ignore,-i[39m[22m        A string containing comma-separated globs that identify
+    [1m[33m--ignore,-i[39m[22m        A string containing semi-colon-separated globs that identify
                        files that should not be checked.
 
     --ignore-files     A comma-separated list of .gitignore-like files that

src/__tests__/check-sync_test.js

@@ -26,6 +26,7 @@ describe("#checkSync", () => {
             {
                 includeGlobs: ["glob1", "glob2"],
                 excludeGlobs: [],
+                ignoreFiles: [],
                 dryRun: true,
                 autoFix: true,
                 comments: ["//"],
@@ -50,6 +51,7 @@ describe("#checkSync", () => {
             {
                 includeGlobs: ["glob1", "glob2"],
                 excludeGlobs: [],
+                ignoreFiles: [],
                 dryRun: false,
                 autoFix: true,
                 comments: ["//"],
@@ -62,6 +64,7 @@ describe("#checkSync", () => {
         expect(getFilesSpy).toHaveBeenCalledWith(
             ["glob1", "glob2"],
             [],
+            [],
             NullLogger,
         );
     });
@@ -74,6 +77,7 @@ describe("#checkSync", () => {
         const options: Options = {
             includeGlobs: ["glob1", "glob2"],
             excludeGlobs: [],
+            ignoreFiles: [],
             dryRun: false,
             autoFix: false,
             comments: ["//"],
@@ -94,6 +98,7 @@ describe("#checkSync", () => {
         const options: Options = {
             includeGlobs: ["glob1", "glob2"],
             excludeGlobs: [],
+            ignoreFiles: [],
             dryRun: false,
             autoFix: false,
             comments: ["//"],
@@ -118,6 +123,7 @@ describe("#checkSync", () => {
         const options: Options = {
             includeGlobs: ["glob1", "glob2"],
             excludeGlobs: [],
+            ignoreFiles: [],
             dryRun: false,
             autoFix: true,
             comments: ["//"],
@@ -146,6 +152,7 @@ describe("#checkSync", () => {
         const options: Options = {
             includeGlobs: ["glob1", "glob2"],
             excludeGlobs: [],
+            ignoreFiles: [],
             dryRun: false,
             autoFix: false,
             comments: ["//"],
@@ -177,6 +184,7 @@ describe("#checkSync", () => {
             {
                 includeGlobs: [],
                 excludeGlobs: [],
+                ignoreFiles: [],
                 autoFix: false,
                 comments: ["//"],
                 dryRun: false,

src/__tests__/cli_test.js

@@ -124,15 +124,15 @@ describe("#run", () => {
         const fakeParsedArgs = {
             ...defaultArgs,
             updateTags: true,
-            comments: "COMMENT1,COMMENT2",
+            comments: "COMMENT1 COMMENT2",
             ignoreFiles: "madeupfile",
+            ignore: "glob1;glob2;",
             _: ["globs", "and globs"],
         };
         const checkSyncSpy = jest
             .spyOn(CheckSync, "default")
             .mockReturnValue({then: jest.fn()});
         jest.spyOn(minimist, "default").mockReturnValue(fakeParsedArgs);
-        jest.spyOn(ParseGitIgnore, "default").mockReturnValue(["madeupglob"]);
 
         // Act
         run(__filename);
@@ -141,7 +141,8 @@ describe("#run", () => {
         expect(checkSyncSpy).toHaveBeenCalledWith(
             {
                 includeGlobs: fakeParsedArgs._,
-                excludeGlobs: ["madeupglob"],
+                excludeGlobs: ["glob1", "glob2"],
+                ignoreFiles: ["madeupfile"],
                 dryRun: false,
                 autoFix: true,
                 comments: ["COMMENT1", "COMMENT2"],
@@ -157,7 +158,7 @@ describe("#run", () => {
         const fakeParsedArgs = {
             ...defaultArgs,
             updateTags: true,
-            comments: "COMMENT1,COMMENT2",
+            comments: "COMMENT1 COMMENT2",
             _: ["globs", "and globs"],
         };
         const checkSyncSpy = jest
@@ -174,6 +175,7 @@ describe("#run", () => {
             {
                 includeGlobs: fakeParsedArgs._,
                 excludeGlobs: [],
+                ignoreFiles: [".gitignore"],
                 dryRun: false,
                 autoFix: true,
                 comments: ["COMMENT1", "COMMENT2"],
@@ -191,7 +193,7 @@ describe("#run", () => {
             updateTags: true,
             ignoreFile: "something",
             noIgnoreFile: true,
-            comments: "COMMENT1,COMMENT2",
+            comments: "COMMENT1 COMMENT2",
             _: ["globs", "and globs"],
         };
         const checkSyncSpy = jest
@@ -208,46 +210,7 @@ describe("#run", () => {
             {
                 includeGlobs: fakeParsedArgs._,
                 excludeGlobs: [],
-                dryRun: false,
-                autoFix: true,
-                comments: ["COMMENT1", "COMMENT2"],
-                json: false,
-                rootMarker: undefined,
-            },
-            expect.any(Object),
-        );
-    });
-
-    it("should combine exclude rules from given ignore files", () => {
-        // Arrange
-        const fakeParsedArgs = {
-            ...defaultArgs,
-            updateTags: true,
-            ignoreFiles: "IGNOREFILEA,IGNOREFILEB",
-            comments: "COMMENT1,COMMENT2",
-            _: ["globs", "and globs"],
-        };
-        const checkSyncSpy = jest
-            .spyOn(CheckSync, "default")
-            .mockReturnValue({then: jest.fn()});
-        jest.spyOn(minimist, "default").mockReturnValue(fakeParsedArgs);
-        jest.spyOn(ParseGitIgnore, "default").mockReturnValueOnce([
-            "IGNORE1",
-            "IGNORE2",
-        ]);
-        jest.spyOn(ParseGitIgnore, "default").mockReturnValueOnce([
-            "IGNORE1",
-            "IGNORE3",
-        ]);
-
-        // Act
-        run(__filename);
-
-        // Assert
-        expect(checkSyncSpy).toHaveBeenCalledWith(
-            {
-                includeGlobs: fakeParsedArgs._,
-                excludeGlobs: ["IGNORE1", "IGNORE2", "IGNORE1", "IGNORE3"],
+                ignoreFiles: [],
                 dryRun: false,
                 autoFix: true,
                 comments: ["COMMENT1", "COMMENT2"],

src/__tests__/fix-file_test.js

@@ -29,6 +29,7 @@ describe("#fixFile", () => {
     const options: Options = {
         includeGlobs: [],
         excludeGlobs: [],
+        ignoreFiles: [],
         dryRun: false,
         autoFix: true,
         comments: [],

src/__tests__/get-files_test.js

@@ -2,75 +2,15 @@
 import * as FastGlob from "fast-glob";
 import Logger from "../logger.js";
 import StringLogger from "../string-logger.js";
+import * as IgnoreFilesToExcludeGlobs from "../ignore-files-to-exclude-globs.js";
 
 import getFiles from "../get-files.js";
+import {jest} from "@jest/globals";
 
 jest.mock("fast-glob");
 jest.mock("fs");
 
 describe("#getFiles", () => {
-    it("should expand foo format includes", async () => {
-        // Arrange
-        const NullLogger = new Logger(null);
-        const globSpy = jest
-            .spyOn(FastGlob, "default")
-            .mockImplementation((pattern, opts) => Promise.resolve([]));
-
-        // Act
-        await getFiles(["foo"], [], NullLogger);
-
-        // Assert
-        expect(globSpy).toHaveBeenCalledWith(
-            ["**/foo/**", "foo"],
-            expect.any(Object),
-        );
-    });
-
-    it("should expand /foo format includes", async () => {
-        // Arrange
-        const NullLogger = new Logger(null);
-        const globSpy = jest
-            .spyOn(FastGlob, "default")
-            .mockImplementation((pattern, opts) => Promise.resolve([]));
-
-        // Act
-        await getFiles(["/foo"], [], NullLogger);
-
-        // Assert
-        expect(globSpy).toHaveBeenCalledWith(
-            ["/foo/**", "/foo"],
-            expect.any(Object),
-        );
-    });
-
-    it("should expand /foo/ format includes", async () => {
-        // Arrange
-        const NullLogger = new Logger(null);
-        const globSpy = jest
-            .spyOn(FastGlob, "default")
-            .mockImplementation((pattern, opts) => Promise.resolve([]));
-
-        // Act
-        await getFiles(["/foo/"], [], NullLogger);
-
-        // Assert
-        expect(globSpy).toHaveBeenCalledWith(["/foo/**"], expect.any(Object));
-    });
-
-    it("should expand foo/ format includes", async () => {
-        // Arrange
-        const NullLogger = new Logger(null);
-        const globSpy = jest
-            .spyOn(FastGlob, "default")
-            .mockImplementation((pattern, opts) => Promise.resolve([]));
-
-        // Act
-        await getFiles(["foo/"], [], NullLogger);
-
-        // Assert
-        expect(globSpy).toHaveBeenCalledWith(["**/foo/**"], expect.any(Object));
-    });
-
     it("should return a sorted list without duplicates", async () => {
         // Arrange
         const NullLogger = new Logger(null);
@@ -81,29 +21,37 @@ describe("#getFiles", () => {
             );
 
         // Act
-        const result = await getFiles(["pattern1", "pattern2"], [], NullLogger);
+        const result = await getFiles(
+            ["pattern1", "pattern2"],
+            [],
+            [],
+            NullLogger,
+        );
 
         // Assert
         expect(result).toEqual(["a", "b", "c", "d"]);
         expect(globSpy).toHaveBeenCalledTimes(1);
     });
 
-    it("should exclude files matched by exclude globs", async () => {
+    it("should exclude files matched by exclude globs and ignore files", async () => {
         // Arrange
         const NullLogger = new Logger(null);
         const globSpy = jest
             .spyOn(FastGlob, "default")
             .mockImplementation((pattern, opts) =>
                 Promise.resolve(["c", "a", "d", "b"]),
             );
+        jest.spyOn(IgnoreFilesToExcludeGlobs, "default").mockReturnValue([
+            "**/ignore-file/**",
+        ]);
 
         // Act
-        await getFiles([], ["a", "c"], NullLogger);
+        await getFiles([], ["a", "c"], ["ignore-file"], NullLogger);
 
         // Assert
         expect(globSpy).toHaveBeenCalledWith(
             [],
-            expect.objectContaining({ignore: ["**/a/**", "a", "**/c/**", "c"]}),
+            expect.objectContaining({ignore: ["a", "c", "**/ignore-file/**"]}),
         );
     });
 
@@ -116,7 +64,7 @@ describe("#getFiles", () => {
         const verboseSpy = jest.spyOn(NullLogger, "verbose");
 
         // Act
-        await getFiles([], ["a", "c"], NullLogger);
+        await getFiles([], ["a", "c"], [], NullLogger);
 
         // Assert
         expect(verboseSpy).toHaveBeenCalledTimes(3);
@@ -130,10 +78,25 @@ describe("#getFiles", () => {
         );
 
         // Act
-        await getFiles([], ["a", "c"], logger);
+        await getFiles(["b", "d"], ["a", "c"], [], logger);
         const log = logger.getLog();
 
         // Assert
-        expect(log).toMatchSnapshot();
+        expect(log).toMatchInlineSnapshot(`
+            " VERBOSE  Include globs: [
+                \\"b\\",
+                \\"d\\"
+            ]
+             VERBOSE  Exclude globs: [
+                \\"a\\",
+                \\"c\\"
+            ]
+             VERBOSE  Discovered paths: [
+                \\"a\\",
+                \\"b\\",
+                \\"c\\",
+                \\"d\\"
+            ]"
+        `);
     });
 });