Skip to content

Commit

Permalink
fix(valid-repository-directory): use repository root for more accurat…
Browse files Browse the repository at this point in the history
…e linting (#498)

<!-- 👋 Hi, thanks for sending a PR to eslint-plugin-package-json! 💖.
Please fill out all fields below and make sure each item is true and [x]
checked.
Otherwise we may not be able to review your PR. -->

## PR Checklist

-   [x] Fixes #252
- [x] That issue was marked as [`status: accepting
prs`](https://github.com/JoshuaKGoldberg/eslint-plugin-package-json/issues?q=is%3Aopen+is%3Aissue+label%3A%22status%3A+accepting+prs%22)
- [x] Steps in
[CONTRIBUTING.md](https://github.com/JoshuaKGoldberg/eslint-plugin-package-json/blob/main/.github/CONTRIBUTING.md)
were taken

## Overview

### Commits

There are two commits:

~1. "settings" - these are just ignores I'm adding to keep my local
development environment out of this project (e.g. I use devbox to manage
my tool chain, so I'm ignoring those files from the repo)~
2. "fix ..." - the actual lint rule fixes

### @altano/repository-tools Dependency

The code changes here aren't that big because we're leaning on my
[@altano/repository-tools](https://www.npmjs.com/package/@altano/repository-tools)
library to do the hardest part: discovering the root of the repository.

Notes about this package:
- SCMs supported: git / mercurial / sapling / subversion
- Performance: the library can find a git repository's root in ~3ms, and
the worst (slowest) repository type (Mercurial) takes ~148ms. I think
this is totally acceptable for package.json linting:

|operation |scm |hz |min |max |mean |p75 |p99 |p995 |p999 |rme |

|------------|------------|-------|-------|-------|-------|-------|-------|-------|-------|--------|
|findRootSync|git |294.36 |3.0661 |5.4844 |3.3972 |3.4789 |4.3417
|5.4844 |5.4844 |±1.37% |
|findRootSync|mercurial |6.7348 |146.39 |152.29 |148.48 |149.20 |152.29
|152.29 |152.29 |±0.91% |
|findRootSync|sapling
|28.1276|33.7934|37.6630|35.5523|36.0934|37.6630|37.6630|37.6630|±1.55%
|
|findRootSync|subversion
|69.9382|13.3270|18.7447|14.2983|14.2685|18.7447|18.7447|18.7447|±2.08%
|
- This repository is well tested. As part of the CI flow it has unit
tests that create real repositories for each supported type and fully
exercise the code.

If you don't want to depend on this library, we can pull in the very
small amount of code directly into this repo, but you'd be losing out on
the really good CI testing.

### Rule Fix

When in a supported repository, the logic for the lint rule is simple:
- Get the repository root path
- We calculate the relative path from the repository root to the
directory of the package.json file being linted
- The package.json `"directory"` field should be that path
- If it isn't, we suggest it
- Example:
  - repository root: "~/src/project"
- package.json file being linted:
"~/src/project/packages/packageA/package.json"
- the expected value for `"directory"` in package.json is
`"packages/packageA"`

When a repository root cannot be found, there is a fallback:
- We make sure the `"directory"` value appears at the end of the
package.json directory path. Since we don't know the root of the
repository, we can't tell how much of the path has to be present. This
can be inaccurate. For example, if package.json is at
'/a/b/c/package.json':
- We make sure `"directory"` is either "/a/b/c" or "b/c" or "c" (these
all validate)
- If you specify a directory that doesn't appear in this path, e.g. 'd',
validation fails
- If you specify something from the middle of the path, e.g. just 'b',
validation fails (we _could_ relax the rule and allow this to pass, but
I can't think of a situation in which this would be valid)
- There are no suggestions in this fallback mode.

Co-authored-by: Josh Goldberg ✨ <[email protected]>
  • Loading branch information
altano and JoshuaKGoldberg authored Sep 12, 2024
1 parent 7aaf31d commit d149400
Show file tree
Hide file tree
Showing 5 changed files with 209 additions and 63 deletions.
1 change: 1 addition & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"pnpm-lock.yaml"
],
"words": [
"altano",
"autofixable",
"Codecov",
"codespace",
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"*": "prettier --ignore-unknown --write"
},
"dependencies": {
"@altano/repository-tools": "^0.1.1",
"detect-indent": "6.1.0",
"detect-newline": "3.1.0",
"package-json-validator": "^0.6.5",
Expand Down
8 changes: 8 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

111 changes: 95 additions & 16 deletions src/rules/valid-repository-directory.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,39 @@
import type { AST as JsonAST } from "jsonc-eslint-parser";

import { findRootSync } from "@altano/repository-tools/findRootSync.cjs";
import * as ESTree from "estree";
import * as path from "node:path";

import { createRule } from "../createRule.js";
import { findPropertyWithKeyValue } from "../utils/findPropertyWithKeyValue.js";

/**
* Checks if the child path appears at the end of the parent path. e.g.
*
* '/a/b/c', 'c' => true
* '/a/b/c', 'b/c' => true
* '/a/b/c', 'b' => false
* '/a/b/c', 'd' => false
*/
function pathEndsWith(parent: string, child: string): boolean {
const segments = parent.split(path.sep);

if (parent === child) {
// the directory specified was the full, absolute path to the
// package.json
return true;
}

// work backwards from the end, adding another path segment to each check
let pathToCheck = "";
return segments.reverse().some((segment) => {
pathToCheck = path.join(segment, pathToCheck);
if (pathToCheck === child) {
return true;
}
});
}

export const rule = createRule({
create(context) {
return {
Expand All @@ -26,24 +54,75 @@ export const rule = createRule({
}

const directoryValue = directoryProperty.value.value;
const expected = path.normalize(path.dirname(context.filename));
const fileDirectory = path.normalize(
path.dirname(context.filename),
);
const repositoryRoot = findRootSync(fileDirectory);

if (repositoryRoot == null) {
// Since we couldn't determine the repository root, we can't
// rigorously determine the validity of the directory value.
// But, we can at least make sure the directory value
// appears at the end of the package.json file path. For
// example:
//
// if /a/b/c/package.json has the value: directory: "b/c"
// it will validate
//
// if /a/b/c/package.json has the value: directory: "b/d"
// it will NOT validate
//
// This ensures that the directory value is at least
// partially correct.
if (
!pathEndsWith(
fileDirectory,
path.normalize(directoryValue),
)
) {
context.report({
messageId: "mismatched",
node: directoryProperty.value as unknown as ESTree.Node,
});
}
} else {
// Get the relative path from repository root to
// package.json. For example:
//
// Simple case (root package.json):
// repositoryRoot: '~/src/project'
// filename: '~/src/project/package.json',
// fileDirectory: '~/src/project',
// expected: '' (directory should be undefined or empty)
//
// Monorepo case (nested package.json):
// repositoryRoot: '~/src/project'
// filename: '~/src/project/packages/packageA/package.json',
// fileDirectory: '~/src/project/packages/packageA',
// expected: 'packages/packageA'
//
const expected = path.relative(
repositoryRoot,
fileDirectory,
);

if (path.normalize(directoryValue) !== expected) {
context.report({
messageId: "mismatched",
node: directoryProperty.value as unknown as ESTree.Node,
suggest: [
{
fix(fixer) {
return fixer.replaceText(
directoryProperty.value as unknown as ESTree.Node,
`"${expected}"`,
);
if (expected !== directoryValue) {
context.report({
messageId: "mismatched",
node: directoryProperty.value as unknown as ESTree.Node,
suggest: [
{
fix(fixer) {
return fixer.replaceText(
directoryProperty.value as unknown as ESTree.Node,
`"${expected}"`,
);
},
messageId: "replace",
},
messageId: "replace",
},
],
});
],
});
}
}
},
};
Expand Down
Loading

0 comments on commit d149400

Please sign in to comment.