[EngSys] decouple filters in changed-files.js

.github/shared/src/changed-files.js

@@ -1,7 +1,8 @@
 // @ts-check
 
 import debug from "debug";
 import { simpleGit } from "simple-git";
+import { includesFolder } from "./path.js";
 
 // Enable simple-git debug logging to improve console output
 debug.enable("simple-git");
@@ -12,17 +13,23 @@
  * @param {string} [options.cwd] Current working directory.  Default: process.cwd().
  * @param {string} [options.headCommitish] Default: "HEAD".
  * @param {import('./logger.js').ILogger} [options.logger]
- * @returns {Promise<string[]>} List of changed files, using posix paths, relative to options.cwd. Example: ["specification/foo/Microsoft.Foo/main.tsp"].
+ * @param {string[]} [options.paths] Limits the diff to the named paths.  If not set, includes all paths in repo.  Default: []
+ * @returns {Promise<string[]>} List of changed files, using posix paths, relative to repo root. Example: ["specification/foo/Microsoft.Foo/main.tsp"].
  */
 export async function getChangedFiles(options = {}) {
-  const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger } = options;
+  const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger, paths = [] } = options;
+
+  if (paths.length > 0) {
+    // Use "--" to separate paths from revisions
+    paths.unshift("--");
+  }
 
   // TODO: If we need to filter based on status, instead of passing an argument to `--diff-filter,
   // consider using "--name-status" instead of "--name-only", and return an array of objects like
   // { name: "/foo/baz.js", status: Status.Renamed, previousName: "/foo/bar.js"}.
   // Then add filter functions to filter based on status.  This is more flexible and lets consumers
   // filter based on status with a single call to `git diff`.
-  const result = await simpleGit(cwd).diff(["--name-only", baseCommitish, headCommitish]);
+  const result = await simpleGit(cwd).diff(["--name-only", baseCommitish, headCommitish, ...paths]);
 
   const files = result.trim().split("\n");
 
@@ -41,11 +48,23 @@
  * @param {string} [options.cwd] Current working directory.  Default: process.cwd().
  * @param {string} [options.headCommitish] Default: "HEAD".
  * @param {import('./logger.js').ILogger} [options.logger]
+ * @param {string[]} [options.paths] Limits the diff to the named paths.  If not set, includes all paths in repo.  Default: []
  * @returns {Promise<{additions: string[], modifications: string[], deletions: string[], renames: {from: string, to: string}[], total: number}>}
  */
 export async function getChangedFilesStatuses(options = {}) {
-  const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger } = options;
-  const result = await simpleGit(cwd).diff(["--name-status", baseCommitish, headCommitish]);
+  const { baseCommitish = "HEAD^", cwd, headCommitish = "HEAD", logger, paths = [] } = options;
+
+  if (paths.length > 0) {
+    // Use "--" to separate paths from revisions
+    paths.unshift("--");
+  }
+
+  const result = await simpleGit(cwd).diff([
+    "--name-status",
+    baseCommitish,
+    headCommitish,
+    ...paths,
+  ]);
 
   const categorizedFiles = {
     additions: /** @type {string[]} */ ([]),
@@ -133,6 +152,7 @@
 }
 
 // Functions suitable for passing to string[].filter(), ordered roughly in order of increasing specificity
+// Functions accept both relative and absolute paths, since paths are resolve()'d before searching (when needed)
 
 /**
  * @param {string} [file]
@@ -152,22 +172,13 @@
   return typeof file === "string" && file.toLowerCase().endsWith("readme.md");
 }
 
-/**
- * @param {string} [file]
- * @returns {boolean}
- */
-export function specification(file) {
-  // Folder name "specification" should match case, since it already exists in repo
-  return typeof file === "string" && file.startsWith("specification/");
-}
-
 /**
  * @param {string} [file]
  * @returns {boolean}
  */
 export function dataPlane(file) {
   // Folder name "data-plane" should match case for consistency across specs
-  return typeof file === "string" && specification(file) && file.includes("/data-plane/");
+  return typeof file === "string" && includesFolder(file, "data-plane");
 }
 
 /**
@@ -176,7 +187,7 @@
  */
 export function resourceManager(file) {
   // Folder name "resource-manager" should match case for consistency across specs
-  return typeof file === "string" && specification(file) && file.includes("/resource-manager/");
+  return typeof file === "string" && includesFolder(file, "resource-manager");
 }
 
 /**
@@ -185,9 +196,7 @@
  */
 export function example(file) {
   // Folder name "examples" should match case for consistency across specs
-  return (
-    typeof file === "string" && json(file) && specification(file) && file.includes("/examples/")
-  );
+  return typeof file === "string" && json(file) && includesFolder(file, "examples");
 }
 
 /**
@@ -197,8 +206,7 @@
 export function typespec(file) {
   return (
     typeof file === "string" &&
-    (file.toLowerCase().endsWith(".tsp") || file.toLowerCase().endsWith("tspconfig.yaml")) &&
-    specification(file)
+    (file.toLowerCase().endsWith(".tsp") || file.toLowerCase().endsWith("tspconfig.yaml"))
   );
 }
 
@@ -221,7 +229,5 @@
  * @returns {boolean}
  */
 export function scenario(file) {
-  return (
-    typeof file === "string" && json(file) && specification(file) && file.includes("/scenarios/")
-  );
+  return typeof file === "string" && json(file) && includesFolder(file, "scenarios");
 }

.github/shared/src/swagger.js

@@ -1,10 +1,10 @@
 // @ts-check
 
 import $RefParser, { ResolverError } from "@apidevtools/json-schema-ref-parser";
 import { readFile } from "fs/promises";
 import { dirname, relative, resolve } from "path";
 import { mapAsync } from "./array.js";
-import { includesFolder } from "./path.js";
+import { example } from "./changed-files.js";
 import { SpecModelError } from "./spec-model-error.js";
 
 /**
@@ -222,26 +222,6 @@
   }
 }
 
-// TODO: Remove duplication with changed-files.js (which currently requires paths relative to repo root)
-
-/**
- * @param {string} [file]
- * @returns {boolean}
- */
-function example(file) {
-  // Folder name "examples" should match case for consistency across specs
-  return typeof file === "string" && json(file) && includesFolder(file, "examples");
-}
-
-/**
- * @param {string} [file]
- * @returns {boolean}
- */
-function json(file) {
-  // Extension "json" with any case is a valid JSON file
-  return typeof file === "string" && file.toLowerCase().endsWith(".json");
-}
-
 // API version lifecycle stages
 export const API_VERSION_LIFECYCLE_STAGES = Object.freeze({
   PREVIEW: "preview",

.github/shared/test/changed-files.test.js

@@ -1,3 +1,3 @@
 // @ts-check
 
 import { afterEach, describe, expect, it, vi } from "vitest";
@@ -8,6 +8,7 @@
   }),
 }));
 
+import { resolve } from "path";
 import * as simpleGit from "simple-git";
 import {
   dataPlane,
@@ -18,7 +19,6 @@
   readme,
   resourceManager,
   scenario,
-  specification,
   swagger,
   typespec,
 } from "../src/changed-files.js";
@@ -47,6 +47,13 @@
     "cspell.yaml",
     "MixedCase.jSoN",
     "README.MD",
+    "not-spec/contosowidgetmanager/data-plane/readme.md",
+    "not-spec/contosowidgetmanager/resource-manager/readme.md",
+    "not-spec/contosowidgetmanager/Contoso.Management/main.tsp",
+    "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml",
+    "not-spec/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json",
+    "not-spec/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
+    "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
     "specification/contosowidgetmanager/data-plane/readme.md",
     "specification/contosowidgetmanager/Contoso.Management/main.tsp",
     "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml",
@@ -57,91 +64,100 @@
     "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
   ];
 
+  const filesResolved = files.map((f) => resolve(f));
+
   it("filter:json", () => {
     const expected = [
       "cspell.json",
       "MixedCase.jSoN",
+      "not-spec/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json",
+      "not-spec/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
+      "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
       "specification/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json",
       "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
       "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json",
       "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
     ];
 
     expect(files.filter(json)).toEqual(expected);
+    expect(filesResolved.filter(json)).toEqual(expected.map((f) => resolve(f)));
   });
 
   it("filter:readme", () => {
     const expected = [
       "README.MD",
+      "not-spec/contosowidgetmanager/data-plane/readme.md",
+      "not-spec/contosowidgetmanager/resource-manager/readme.md",
       "specification/contosowidgetmanager/data-plane/readme.md",
       "specification/contosowidgetmanager/resource-manager/readme.md",
     ];
 
     expect(files.filter(readme)).toEqual(expected);
-  });
-
-  it("filter:specification", () => {
-    const expected = [
-      "specification/contosowidgetmanager/data-plane/readme.md",
-      "specification/contosowidgetmanager/Contoso.Management/main.tsp",
-      "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml",
-      "specification/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json",
-      "specification/contosowidgetmanager/resource-manager/readme.md",
-      "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
-      "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json",
-      "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
-    ];
-
-    expect(files.filter(specification)).toEqual(expected);
+    expect(filesResolved.filter(readme)).toEqual(expected.map((f) => resolve(f)));
   });
 
   it("filter:typespec", () => {
     const expected = [
+      "not-spec/contosowidgetmanager/Contoso.Management/main.tsp",
+      "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml",
       "specification/contosowidgetmanager/Contoso.Management/main.tsp",
       "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml",
     ];
     expect(files.filter(typespec)).toEqual(expected);
   });
 
   it("filter:data-plane", () => {
-    const expected = ["specification/contosowidgetmanager/data-plane/readme.md"];
+    const expected = [
+      "not-spec/contosowidgetmanager/data-plane/readme.md",
+      "specification/contosowidgetmanager/data-plane/readme.md",
+    ];
 
     expect(files.filter(dataPlane)).toEqual(expected);
+    expect(filesResolved.filter(dataPlane)).toEqual(expected.map((f) => resolve(f)));
   });
 
   it("filter:resource-manager", () => {
     const expected = [
+      "not-spec/contosowidgetmanager/resource-manager/readme.md",
+      "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
       "specification/contosowidgetmanager/resource-manager/readme.md",
       "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
       "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json",
     ];
 
     expect(files.filter(resourceManager)).toEqual(expected);
+    expect(filesResolved.filter(resourceManager)).toEqual(expected.map((f) => resolve(f)));
   });
 
   it("filter:example", () => {
     const expected = [
+      "not-spec/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json",
       "specification/contosowidgetmanager/Contoso.Management/examples/2021-11-01/Employees_Get.json",
       "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/examples/Employees_Get.json",
     ];
 
     expect(files.filter(example)).toEqual(expected);
+    expect(filesResolved.filter(example)).toEqual(expected.map((f) => resolve(f)));
   });
 
   it("filter:scenarios", () => {
     const expected = [
+      "not-spec/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
       "specification/contosowidgetmanager/Contoso.Management/scenarios/2021-11-01/Employees_Get.json",
     ];
 
     expect(files.filter(scenario)).toEqual(expected);
+    expect(filesResolved.filter(scenario)).toEqual(expected.map((f) => resolve(f)));
   });
 
   it("filter:swagger", () => {
     const expected = [
+      "not-spec/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
       "specification/contosowidgetmanager/resource-manager/Microsoft.Contoso/stable/2021-11-01/contoso.json",
     ];
 
     expect(files.filter(swagger)).toEqual(expected);
+    expect(filesResolved.filter(swagger)).toEqual(expected.map((f) => resolve(f)));
   });
 
   describe("getChangedFilesStatuses", () => {

.github/workflows/lintdiff-code.yaml

@@ -1,3 +1,3 @@
 name: "Swagger LintDiff - Analyze Code"
 
 on: pull_request
@@ -42,7 +42,7 @@
             const { join } = await import('path');
 
             // TODO: Logger
-            const changedFiles = await getChangedFiles({ cwd: 'after'});
+            const changedFiles = await getChangedFiles({ cwd: 'after', paths: ['specification'] });
             console.log('Changed files:', changedFiles);
 
             const filePath = join(process.cwd(), 'changed-files.txt');

.github/workflows/src/arm-incremental-typespec.js

@@ -1,3 +1,3 @@
 // @ts-check
 
 // For now, treat all paths as posix, since this is the format returned from git commands
@@ -24,6 +24,7 @@
 export default async function incrementalTypeSpec({ core }) {
   const options = {
     cwd: process.env.GITHUB_WORKSPACE,
+    paths: ["specification"],
     logger: new CoreLogger(core),
   };
 

eng/tools/lint-diff/src/processChanges.ts

@@ -1,7 +1,7 @@
 import { join, relative, resolve, sep } from "path";
 import { readFile } from "fs/promises";
 import { pathExists } from "./util.js";
-import { specification, readme, swagger } from "@azure-tools/specs-shared/changed-files";
+import { readme, swagger } from "@azure-tools/specs-shared/changed-files";
 import { SpecModel } from "@azure-tools/specs-shared/spec-model";
 import { ReadmeAffectedTags } from "./lintdiff-types.js";
 import deepEqual from "deep-eql";
@@ -19,12 +19,9 @@
 
   // Read changed files, exclude any files that should be ignored
   const ignoreFilesWith = ["/examples/", "/quickstart-templates/", "/scenarios/"];
-  const changedSpecFiles = (await readFileList(changedFilesPath)).filter((file) => {
-    // File is in specification/ folder
-    if (!specification(file)) {
-      return false;
-    }
 
+  // Changed files should already be filtered to the top-level "specification" folder (see lintdiff-code.yaml)
+  const changedSpecFiles = (await readFileList(changedFilesPath)).filter((file) => {
     // File is not ignored
     for (const ignore of ignoreFilesWith) {
       if (file.includes(ignore)) {