Skip to content
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

feat: add the "valid-license" rule #786

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 76 additions & 0 deletions docs/rules/valid-license.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
# valid-license

💼 This rule is enabled in the ✅ `recommended` config.

<!-- end auto-generated rule header -->

This rule applies two validations to the `"license"` property:

- It must be a string rather than any other data type
- It's value should match one of the values provided in the options

Example of **incorrect** code for this rule:

When the rule is configured with

```json
{
"valid-license": ["error", "GPL"]
}
```

```json
{
"license": "MIT"
}
```

When the rule is configured with

```json
{
"valid-license": ["error", ["MIT", "GPL"]]
}
```

```json
{
"license": "Apache"
}
```

Example of **correct** code for this rule:

When the rule is configured with

```json
{
"valid-license": ["error", "GPL"]
}
```

```json
{
"license": "GPL"
}
```

When the rule is configured with

```json
{
"valid-license": ["error", ["Apache", "MIT"]]
}
```

```json
{
"license": "Apache"
}
```

```json
{
"license": "MIT"
}
```
70 changes: 36 additions & 34 deletions src/plugin.ts
xenobytezero marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { rule as orderProperties } from "./rules/order-properties.js";
import { rule as preferRepositoryShorthand } from "./rules/repository-shorthand.js";
import { rule as sortCollections } from "./rules/sort-collections.js";
import { rule as uniqueDependencies } from "./rules/unique-dependencies.js";
import { rule as validLicense } from "./rules/valid-license.js";
import { rule as validLocalDependency } from "./rules/valid-local-dependency.js";
import { rule as validName } from "./rules/valid-name.js";
import { rule as validPackageDefinition } from "./rules/valid-package-definition.js";
Expand All @@ -17,48 +18,49 @@ import { rule as validVersion } from "./rules/valid-version.js";
const require = createRequire(import.meta.url || __filename);

const { name, version } = require("../package.json") as {
name: string;
version: string;
name: string;
version: string;
};

const rules: Record<string, PackageJsonRuleModule> = {
"no-empty-fields": noEmptyFields,
"no-redundant-files": noRedundantFiles,
"order-properties": orderProperties,
"repository-shorthand": preferRepositoryShorthand,
"sort-collections": sortCollections,
"unique-dependencies": uniqueDependencies,
"valid-local-dependency": validLocalDependency,
"valid-name": validName,
"valid-package-definition": validPackageDefinition,
"valid-repository-directory": validRepositoryDirectory,
"valid-version": validVersion,
"no-empty-fields": noEmptyFields,
"no-redundant-files": noRedundantFiles,
"order-properties": orderProperties,
"repository-shorthand": preferRepositoryShorthand,
"sort-collections": sortCollections,
"unique-dependencies": uniqueDependencies,
"valid-license": validLicense,
"valid-local-dependency": validLocalDependency,
"valid-name": validName,
"valid-package-definition": validPackageDefinition,
"valid-repository-directory": validRepositoryDirectory,
"valid-version": validVersion,

/** @deprecated use 'valid-package-definition' instead */
"valid-package-def": {
...validPackageDefinition,
meta: {
...validPackageDefinition.meta,
deprecated: true,
docs: {
...validPackageDefinition.meta.docs,
recommended: false,
},
replacedBy: ["valid-package-definition"],
},
},
/** @deprecated use 'valid-package-definition' instead */
"valid-package-def": {
...validPackageDefinition,
meta: {
...validPackageDefinition.meta,
deprecated: true,
docs: {
...validPackageDefinition.meta.docs,
recommended: false,
},
replacedBy: ["valid-package-definition"],
},
},
};

export const plugin = {
meta: {
name,
version,
},
rules,
meta: {
name,
version,
},
rules,
};

export const recommendedRuleSettings = Object.fromEntries(
Object.entries(rules)
.filter(([, rule]) => rule.meta.docs?.recommended)
.map(([name]) => ["package-json/" + name, "error" as const]),
Object.entries(rules)
.filter(([, rule]) => rule.meta.docs?.recommended)
.map(([name]) => ["package-json/" + name, "error" as const]),
);
80 changes: 80 additions & 0 deletions src/rules/valid-license.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import type { AST as JsonAST } from "jsonc-eslint-parser";

import * as ESTree from "estree";

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

export type AllowedValues = string | string[];
export type Options = [AllowedValues];

export function generateReportData(allowed: string[]) {
return {
allowed: allowed.map((v) => `'${v}'`).join(","),
multiple: allowed.length > 1 ? "one of" : "",
};
}

export const rule = createRule<Options>({
create(context) {
const ruleOptions = context.options[0];
const allowedValues = Array.isArray(ruleOptions)
? ruleOptions
: [ruleOptions];
return {
"Program > JSONExpressionStatement > JSONObjectExpression > JSONProperty[key.value=license]"(
node: JsonAST.JSONProperty,
) {
if (
node.value.type !== "JSONLiteral" ||
typeof node.value.value !== "string"
) {
context.report({
messageId: "nonString",
node: node.value as unknown as ESTree.Node,
});
return;
}

if (!allowedValues.includes(node.value.value)) {
context.report({
data: generateReportData(allowedValues),
loc: node.loc,
messageId: "invalidValue",
});
}
},
};
},

meta: {
docs: {
category: "Best Practices",
description:
"Enforce the 'license' field to be a specific value/values",
recommended: false,
},
messages: {
invalidValue: '"license" must be{{multiple}} {{allowed}}"',
nonString: '"license" must be a string"',
},
schema: {
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg Feb 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Feature] If this rule is to be enabled in the recommended config -which would be great!- then we're going to need it to not be prescriptive on which of the "valid" license(s) are allowed. Any "valid" license should be allowed unless the user has customized the rule. "Valid" here could mean one of several things, depending on how strict the user wants to be. In increasing order of strictness...

  1. 🍏 Any string value
  2. 🍌 Any SPDX license ID (on npm: spdx-license-ids) or SPDX license expression syntax referring to SPDX license IDs
  3. 🌸 The same as 2., but only OSI approved license IDs
  4. 🐉 The same as 2., but only FSF Free/Libre approved license IDs
  5. 🥚 >=1 specific license IDs (what the PR has right now)

I think any of 2 🍌, 3 🌸, 4 🐉, and 5 🥚 would be fine as followup issues (with the caveat that 3 🌸 and 4 🐉 depend on 2 🍌). Most users would be satisfied with just 1 🍏 checking that it's a string.

But, the options right now don't allow 1 🍏. They require 5 🥚, passing in a specific list of IDs. Could you please make the 5 🥚 specific license ID checking optional? As in, if the user doesn't provide any licenses, the rule should only check 1 🍏 that it's a valid string.

Reference: https://docs.npmjs.com/cli/v11/configuring-npm/package-json#license

items: [
{
oneOf: [
{
items: {
type: "string",
},
type: "array",
},
{
type: "string",
},
],
},
],
type: "array",
},
type: "problem",
},
});
74 changes: 74 additions & 0 deletions src/tests/rules/valid-license.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { generateReportData, rule } from "../../rules/valid-license.js";
import { ruleTester } from "./ruleTester.js";

ruleTester.run("valid-license", rule, {
invalid: [
// Invalid with single valid value
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs] Nit: IMO these comments aren't worth keeping up-to-date. ESLint rule tests tend to get very nuanced and tricky over time. I've seen suits with many dozens or even hundreds of tests that exercise various edge cases on themes. Personally I'd remove this:

Suggested change
// Invalid with single valid value

If you really want to include a name per test, the proper way would be with a name property per https://eslint.org/docs/latest/integrate/nodejs-api#ruletester.

Not a blocker, just a preference. 🙂

{
code: JSON.stringify({
license: "CC BY-SA",
name: "some-test-package",
}),
errors: [
{
data: generateReportData(["GPL"]),
messageId: "invalidValue",
},
],
options: ["GPL"],
},
// Invalid with multiple valid values
{
code: JSON.stringify({
license: "Apache",
name: "some-test-package",
}),
errors: [
{
data: generateReportData(["MIT", "GPL"]),
messageId: "invalidValue",
},
],
options: [["MIT", "GPL"]],
},
// Invalid property type
{
code: JSON.stringify({
license: 1234,
name: "some-test-package",
}),
errors: [
{
data: generateReportData(["GPL"]),
messageId: "nonString",
},
],
options: ["GPL"],
},
],
valid: [
// Valid value from single valid value
{
code: JSON.stringify({
license: "Apache",
name: "some-test-package",
}),
options: ["Apache"],
},
// Valid value from multiple valid values
{
code: JSON.stringify({
license: "GPL",
name: "some-test-package",
}),
options: [["MIT", "GPL"]],
},
// Missing property
{
code: JSON.stringify({
name: "some-test-package",
}),
options: [["MIT", "GPL"]],
},
],
});