fix: clean up man sorting

wraithgar · wraithgar · commit 6a4bcbaaf12c · 2023-03-23T09:35:51.000-07:00
glob doesn't sort and returns things in the os specific representation.
Also a lot of the edge cases here covered non-reality.  Our current man
setup already has all of the `npm-` prefixed things in `man1`.
diff --git a/lib/commands/help.js b/lib/commands/help.js
@@ -11,8 +11,6 @@ const BaseCommand = require('../base-command.js')
 // We don't currently compress our man pages but if we ever did this would
 // seamlessly continue supporting it
 const manNumberRegex = /\.(\d+)(\.[^/\\]*)?$/
-// Searches for the "npm-" prefix in page names, to prefer those.
-const manNpmPrefixRegex = /\/npm-/
 // hardcoded names for mansections
 // XXX: these are used in the docs workspace and should be exported
 // from npm so section names can changed more easily
@@ -66,33 +64,13 @@ class Help extends BaseCommand {
     const f = globify(path.resolve(this.npm.npmRoot, `man/${manSearch}/?(npm-)${arg}.[0-9]*`))
 
     const [man] = await glob(f).then(r => r.sort((a, b) => {
-      // Prefer the page with an npm prefix, if there's only one.
-      const aHasPrefix = manNpmPrefixRegex.test(a)
-      const bHasPrefix = manNpmPrefixRegex.test(b)
-      if (aHasPrefix !== bHasPrefix) {
-        /* istanbul ignore next */
-        return aHasPrefix ? -1 : 1
-      }
-
       // Because the glob is (subtly) different from manNumberRegex,
       // we can't rely on it passing.
-      const aManNumberMatch = a.match(manNumberRegex)
-      const bManNumberMatch = b.match(manNumberRegex)
-      if (aManNumberMatch) {
-        /* istanbul ignore next */
-        if (!bManNumberMatch) {
-          return -1
-        }
-        // man number sort first so that 1 aka commands are preferred
-        if (aManNumberMatch[1] !== bManNumberMatch[1]) {
-          return aManNumberMatch[1] - bManNumberMatch[1]
-        }
-        /* istanbul ignore next */
-      } else if (bManNumberMatch) {
-        /* istanbul ignore next */
-        return 1
+      const aManNumberMatch = a.match(manNumberRegex)?.[1] || 999
+      const bManNumberMatch = b.match(manNumberRegex)?.[1] || 999
+      if (aManNumberMatch !== bManNumberMatch) {
+        return aManNumberMatch - bManNumberMatch
       }
-
       return localeCompare(a, b)
     }))
 
diff --git a/test/lib/commands/help.js b/test/lib/commands/help.js
@@ -29,8 +29,8 @@ const genManPages = (obj) => {
 
 const mockHelp = async (t, {
   man = {
-    1: ['whoami', 'install', 'star', 'unstar', 'uninstall', 'unpublish'].map(p => `npm-${p}`),
     5: ['npmrc', 'install', 'package-json'],
+    1: ['whoami', 'install', 'star', 'unstar', 'uninstall', 'unpublish'].map(p => `npm-${p}`),
     7: ['disputes', 'config'],
   },
   browser = false,
@@ -113,7 +113,7 @@ t.test('npm help whoami', async t => {
   const [spawnBin, spawnArgs] = getArgs()
   t.equal(spawnBin, 'man', 'calls man by default')
   t.equal(spawnArgs.length, 1)
-  t.match(spawnArgs[0], /\/man\/man1\/npm-whoami\.1$/)
+  t.match(spawnArgs[0], /npm-whoami\.1$/)
 })
 
 t.test('npm help 1 install', async t => {
@@ -155,7 +155,7 @@ t.test('npm help package.json redirects to package-json', async t => {
   const [spawnBin, spawnArgs] = getArgs()
   t.equal(spawnBin, 'man', 'calls man by default')
   t.equal(spawnArgs.length, 1)
-  t.match(spawnArgs[0], /\/man\/man5\/package-json\.5$/)
+  t.match(spawnArgs[0], /package-json\.5$/)
 })
 
 t.test('npm help ?(un)star', async t => {
@@ -168,7 +168,7 @@ t.test('npm help ?(un)star', async t => {
   t.equal(spawnBin, 'emacsclient', 'maps woman to emacs correctly')
   t.equal(spawnArgs.length, 2)
   t.match(spawnArgs[1], /^\(woman-find-file '/)
-  t.match(spawnArgs[1], /\/man\/man1\/npm-star.1'\)$/)
+  t.match(spawnArgs[1], /npm-star.1'\)$/)
 })
 
 t.test('npm help un*', async t => {
@@ -179,39 +179,40 @@ t.test('npm help un*', async t => {
   const [spawnBin, spawnArgs] = getArgs()
   t.equal(spawnBin, 'man', 'calls man by default')
   t.equal(spawnArgs.length, 1)
-  t.match(spawnArgs[0], /\/man\/man1\/npm-uninstall\.1$/)
+  t.match(spawnArgs[0], /npm-uninstall\.1$/)
 })
 
-t.test('npm help - prefers npm help pages', async t => {
+t.test('npm help - prefers lowest numbered npm prefixed help pages', async t => {
   const { getArgs } = await mockHelp(t, {
     man: {
       6: ['npm-install'],
-      1: ['install'],
-      5: ['install', 'npm-install'],
+      1: ['npm-install'],
+      5: ['install'],
+      7: ['npm-install'],
     },
     exec: ['install'],
   })
 
   const [spawnBin, spawnArgs] = getArgs()
   t.equal(spawnBin, 'man', 'calls man by default')
   t.equal(spawnArgs.length, 1)
-  t.match(spawnArgs[0], /\/man\/man5\/npm-install\.5$/)
+  t.match(spawnArgs[0], /npm-install\.1$/)
 })
 
 t.test('npm help - works in the presence of strange man pages', async t => {
   const { getArgs } = await mockHelp(t, {
     man: {
-      '6strange': ['config'],
-      1: ['config'],
-      '5ssl': ['config'],
+      '1strange': ['config'],
+      5: ['config'],
+      '6ssl': ['config'],
     },
     exec: ['config'],
   })
 
   const [spawnBin, spawnArgs] = getArgs()
   t.equal(spawnBin, 'man', 'calls man by default')
   t.equal(spawnArgs.length, 1)
-  t.match(spawnArgs[0], /\/man\/man1\/config\.1$/)
+  t.match(spawnArgs[0], /config\.5$/)
 })
 
 t.test('rejects with code', async t => {
diff --git a/workspaces/config/tap-snapshots/test/type-description.js.test.cjs b/workspaces/config/tap-snapshots/test/type-description.js.test.cjs
@@ -10,6 +10,11 @@ Object {
   "_exit": Array [
     "boolean value (true or false)",
   ],
+  "_single": Array [
+    Object {
+      "exotic": "not part of normal typedefs",
+    },
+  ],
   "access": Array [
     null,
     "restricted",
diff --git a/workspaces/config/test/fixtures/types.js b/workspaces/config/test/fixtures/types.js
@@ -148,4 +148,5 @@ module.exports = {
   versions: Boolean,
   viewer: String,
   _exit: Boolean,
+  _single: { exotic: 'not part of normal typedefs' },
 }

Original file line number	Diff line number	Diff line change
`@@ -148,4 +148,5 @@ module.exports = {`
`148`	`148`	`versions: Boolean,`
`149`	`149`	`viewer: String,`
`150`	`150`	`_exit: Boolean,`
	`151`	`+ _single: { exotic: 'not part of normal typedefs' },`
`151`	`152`	`}`