Update name-format to cover all cases (#537)

* Update format-name rule to check for all name errors

* Update rule documentation

Co-authored-by: Thomas Lindner <[email protected]>

Author: aarongoldenthal

docs/rules/format/name-format.md

@@ -3,7 +3,7 @@ id: name-format
 title: name-format
 ---
 
-Enabling this rule will result in an error being generated if `name` is not lowercase.
+Enabling this rule will result in an error being generated if `name` does not meet the [naming constraints](https://github.com/npm/validate-npm-package-name#naming-rules).
 
 ## Example .npmpackagejsonlintrc configuration
 
@@ -25,6 +25,18 @@ Enabling this rule will result in an error being generated if `name` is not lowe
 }
 ```
 
+```json
+{
+  "name": "npm package json lint"
+}
+```
+
+```json
+{
+  "name": ".npm-package-json-lint"
+}
+```
+
 ### *Correct* example(s)
 
 ```json
@@ -35,4 +47,6 @@ Enabling this rule will result in an error being generated if `name` is not lowe
 
 ## History
 
+* Augmented with all name checks in `validate-npm-package-name` in version 6.0.0
+* Added checks for name length and not starting with . or _ in version 4.0.0
 * Introduced in version 1.0.0

package-lock.json

@@ -51,7 +51,8 @@
     "plur": "^4.0.0",
     "semver": "^7.3.5",
     "slash": "^3.0.0",
-    "strip-json-comments": "^3.1.1"
+    "strip-json-comments": "^3.1.1",
+    "validate-npm-package-name": "^3.0.0"
   },
   "devDependencies": {
     "eslint": "^7.32.0",

package.json

@@ -1,28 +1,21 @@
-const {isLowercase} = require('../validators/format');
+const validateName = require('validate-npm-package-name');
 const LintIssue = require('../LintIssue');
+const getNameError = require('../utils/getNameError');
 
 const lintId = 'name-format';
 const nodeName = 'name';
 const ruleType = 'standard';
-const maxLength = 214;
 
 const lint = (packageJsonData, severity) => {
   if (!packageJsonData.hasOwnProperty(nodeName)) {
     return true;
   }
 
   const name = packageJsonData[nodeName];
+  const results = validateName(name);
 
-  if (!isLowercase(name)) {
-    return new LintIssue(lintId, severity, nodeName, 'Format should be all lowercase');
-  }
-
-  if (name.length > maxLength) {
-    return new LintIssue(lintId, severity, nodeName, `name should be less than or equal to ${maxLength} characters.`);
-  }
-
-  if (name.startsWith('.') || name.startsWith('_')) {
-    return new LintIssue(lintId, severity, nodeName, 'name should not start with . or _');
+  if (!results.validForNewPackages) {
+    return new LintIssue(lintId, severity, nodeName, getNameError(results));
   }
 
   return true;

src/rules/name-format.js

@@ -0,0 +1,22 @@
+/**
+ * Gets the error message indicating why the name is invalid.
+ *
+ * @param   {object} results Results from validate-npm-package-name
+ * @returns {string}         Error/warning message
+ */
+const getNameError = (results) => {
+  // Errors are returned for names that were never valid
+  if (results.errors && results.errors.length > 0) {
+    return results.errors[0];
+  }
+
+  // Warnings are returned for names that are no longer valid
+  if (results.warnings && results.warnings.length > 0) {
+    return results.warnings[0];
+  }
+
+  // Ensure that an error message is returned in any case
+  return 'name invalid';
+};
+
+module.exports = getNameError;

src/utils/getNameError.js

@@ -1,12 +1,5 @@
 const semver = require('semver');
 
-/**
- * Determines whether or not the string is lowercase
- * @param  {string} name  Name
- * @return {boolean}      True if the string is lowercase or is missing. False if it is not.
- */
-const isLowercase = (name) => name === name.toLowerCase();
-
 /**
  * Determines whether or not the node's value is a valid semantic version
  * @param  {object} packageJsonData Valid JSON
@@ -22,6 +15,5 @@ const isValidVersionNumber = (packageJsonData, nodeName) => {
 };
 
 module.exports = {
-  isLowercase,
   isValidVersionNumber,
 };

src/validators/format.js

@@ -19,22 +19,34 @@ describe('name-format Unit Tests', () => {
       expect(response.lintId).toStrictEqual('name-format');
       expect(response.severity).toStrictEqual('error');
       expect(response.node).toStrictEqual('name');
-      expect(response.lintMessage).toStrictEqual('Format should be all lowercase');
+      expect(response.lintMessage).toStrictEqual('name can no longer contain capital letters');
+    });
+
+    test('contains space - LintIssue object should be returned', () => {
+      const packageJsonData = {
+        name: 'contains space',
+      };
+      const response = lint(packageJsonData, 'error');
+
+      expect(response.lintId).toStrictEqual('name-format');
+      expect(response.severity).toStrictEqual('error');
+      expect(response.node).toStrictEqual('name');
+      expect(response.lintMessage).toStrictEqual('name can only contain URL-friendly characters');
     });
 
     test('exceeds max length - LintIssue object should be returned', () => {
       const packageJsonData = {
-        name: 'a'.padStart(215),
+        name: 'a'.padStart(215, 'a'),
       };
       const response = lint(packageJsonData, 'error');
 
       expect(response.lintId).toStrictEqual('name-format');
       expect(response.severity).toStrictEqual('error');
       expect(response.node).toStrictEqual('name');
-      expect(response.lintMessage).toStrictEqual('name should be less than or equal to 214 characters.');
+      expect(response.lintMessage).toStrictEqual('name can no longer contain more than 214 characters');
     });
 
-    test('starts with . - LintIssue object should be returned', () => {
+    test('starts with . and no scope - LintIssue object should be returned', () => {
       const packageJsonData = {
         name: '.lowercase',
       };
@@ -43,10 +55,10 @@ describe('name-format Unit Tests', () => {
       expect(response.lintId).toStrictEqual('name-format');
       expect(response.severity).toStrictEqual('error');
       expect(response.node).toStrictEqual('name');
-      expect(response.lintMessage).toStrictEqual('name should not start with . or _');
+      expect(response.lintMessage).toStrictEqual('name cannot start with a period');
     });
 
-    test('starts with _ - LintIssue object should be returned', () => {
+    test('starts with _ and no scope - LintIssue object should be returned', () => {
       const packageJsonData = {
         name: '_lowercase',
       };
@@ -55,7 +67,36 @@ describe('name-format Unit Tests', () => {
       expect(response.lintId).toStrictEqual('name-format');
       expect(response.severity).toStrictEqual('error');
       expect(response.node).toStrictEqual('name');
-      expect(response.lintMessage).toStrictEqual('name should not start with . or _');
+      expect(response.lintMessage).toStrictEqual('name cannot start with an underscore');
+    });
+  });
+
+  describe('when package.json has node with correct format', () => {
+    test('all valid characters - true should be returned', () => {
+      const packageJsonData = {
+        name: 'lowercase-name',
+      };
+      const response = lint(packageJsonData, 'error');
+
+      expect(response).toBe(true);
+    });
+
+    test('starts with . and has scope - true should be returned', () => {
+      const packageJsonData = {
+        name: '@foo/.lowercase',
+      };
+      const response = lint(packageJsonData, 'error');
+
+      expect(response).toBe(true);
+    });
+
+    test('starts with _ and has scope - true should be returned', () => {
+      const packageJsonData = {
+        name: '@foo/_lowercase',
+      };
+      const response = lint(packageJsonData, 'error');
+
+      expect(response).toBe(true);
     });
   });
 

test/unit/rules/name-format.test.js

@@ -0,0 +1,38 @@
+const getNameError = require('../../../src/utils/getNameError');
+
+const genericErrorMessage = 'name invalid';
+const getResults = (includeErrors = true, includeWarnings = true) => {
+  const results = {
+    validForNewPackages: false,
+    validForOldPackages: !includeErrors,
+    warnings: ['first warning', 'second warning'],
+    errors: ['first error', 'second error'],
+  };
+
+  if (!includeErrors) {
+    delete results.errors;
+  }
+
+  if (!includeWarnings) {
+    delete results.warnings;
+  }
+
+  return results;
+};
+
+describe('getNameError Unit Tests', () => {
+  test('if errors - returns first error', () => {
+    const results = getResults(true, true);
+    expect(getNameError(results)).toBe(results.errors[0]);
+  });
+
+  test('if warnings and no errors - returns first warning', () => {
+    const results = getResults(false, true);
+    expect(getNameError(results)).toBe(results.warnings[0]);
+  });
+
+  test('if no warnings and no errors - returns generic error message', () => {
+    const results = getResults(false, false);
+    expect(getNameError(results)).toBe(genericErrorMessage);
+  });
+});

test/unit/utils/getNameError.test.js

@@ -1,26 +1,6 @@
 const format = require('../../../src/validators/format');
 
 describe('format Unit Tests', () => {
-  describe('isLowercase method', () => {
-    describe('when the string is lowercase', () => {
-      test('true should be returned', () => {
-        const string = 'awesome-module';
-        const response = format.isLowercase(string);
-
-        expect(response).toBe(true);
-      });
-    });
-
-    describe('when the string is not lowercase', () => {
-      test('false should be returned', () => {
-        const string = 'aweSome-moDule';
-        const response = format.isLowercase(string);
-
-        expect(response).toBe(false);
-      });
-    });
-  });
-
   describe('isValidVersionNumber method', () => {
     describe('when the node does not exist in the package.json file', () => {
       test('true should be returned', () => {