Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

use common npm abbreviations, fixes #435 #484

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bronson
Copy link
Contributor

@bronson bronson commented Dec 29, 2015

Fixes #435

Also updated some tests to use the abbreviations where convenient.

src/view.coffee Outdated
@@ -9,7 +9,7 @@ tree = require './tree'

module.exports =
class View extends Command
@commandNames: ['view', 'show']
@commandNames: ['view', 'show', 'v']
Copy link

Choose a reason for hiding this comment

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

Shouldn't info be added as an alias?

Homebrew uses it, and I'm constantly getting info mixed up with show or view...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(more than a year later...)

Sounds good, included.

@bronson
Copy link
Contributor Author

bronson commented Oct 14, 2016

Ha, sure. Added @Alhadis's suggestion and rebased to master.

@bronson
Copy link
Contributor Author

bronson commented Oct 14, 2016

Alas, it looks like CI on master is broken. The failure isn't related to this patch.

@joefitzgerald
Copy link
Contributor

I would much prefer that new specs are added for the abbreviations, rather than changing the existing command names (e.g. install > i). This will ensure there are not regressions for the full command names.

@bronson
Copy link
Contributor Author

bronson commented Sep 4, 2017

Heh, maybe no tests needed? 94a730e

@bronson bronson force-pushed the abbreviations branch 3 times, most recently from 167e92d to 9388627 Compare September 4, 2017 22:33
@bronson
Copy link
Contributor Author

bronson commented Sep 4, 2017

OK, the patch has been updated to just specify the abbreviations, not test them.

Agreed, it would be nice to have specs for the abbreviations. I don't see a way to do it that looks clean though... Best I came up with would be to write tests like this:

describe 'aliases for install', ->
  runs ->
    expect(apm.findCommand(['i'])).toBe Install

Which requires the following patch to apm-cli.coffee:

diff --git a/src/apm-cli.coffee b/src/apm-cli.coffee
index fde6865..ff0a647 100644
--- a/src/apm-cli.coffee
+++ b/src/apm-cli.coffee
@@ -154,7 +154,12 @@ getPythonVersion = (callback) ->
         version = version?.trim()
       callback(version)
 
+findCommand = (command) ->
+  commands[command]
+
 module.exports =
+  findCommand: findCommand
+
   run: (args, callback) ->
     config.setupApmRcFile()
     options = parseOptions(args)
@@ -203,7 +208,7 @@ module.exports =
         else
           showHelp(options)
         options.callback()
-      else if Command = commands[command]
+      else if Command = findCommand(command)
         new Command().run(options)
       else
         options.callback("Unrecognized command: #{command}")

Can finish doing that if you think it's worth it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants