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

[rush] Improve command-line for "rush add" #1160

Closed
qm3ster opened this issue Mar 18, 2019 · 7 comments · Fixed by #2459
Closed

[rush] Improve command-line for "rush add" #1160

qm3ster opened this issue Mar 18, 2019 · 7 comments · Fixed by #2459
Labels
enhancement The issue is asking for a new feature or design change

Comments

@qm3ster
Copy link

qm3ster commented Mar 18, 2019

Not sure why rush add takes packages through the -p flag and not just naked parameters like all the package managers do, but it should still be able to install/update version of multiple package identifiers at once, for example with multiple flags: -p PACKAGE -p PACKAGE@VERSION.

The current behavior when you supply rush add -p foo -p bar is particularly surprising - instead of erroring out since this isn't supported, it just ignores the package names before the last one supplied. You end up only adding bar. That's probably a Bad Thing™

@octogonz
Copy link
Collaborator

It's a limitation of the ts-command-line library. #790 is a similar issue. For a long time we've been meaning to replace the underlying parser that ts-command-line uses, but it hasn't been prioritized because we actually don't receive a lot of complaints about it compared to other things. :-)

@octogonz octogonz added the enhancement The issue is asking for a new feature or design change label Mar 19, 2019
@octogonz octogonz changed the title [rush] rush add multiple packages [rush] Improve command-line for "rush add" Mar 19, 2019
@domarmstrong
Copy link
Contributor

Can I add another complaint them :) this has been a pain for myself also. The -p flag seems pretty redundant. And having to rush add -p foo then wait, then add another, when you have 5 packages or so to add!

@octogonz
Copy link
Collaborator

octogonz commented Jun 2, 2019

And having to rush add -p foo then wait, then add another, when you have 5 packages or so to add!

You can skip the waiting by using --skip-update, like this:

# Skip updates
$ rush add -p lib1 -s
$ rush add -p lib2 -s
$ rush add -p lib3 -s
$ rush add -p lib4 -s
# Perform one update for all 5 additions
$ rush add -p lib5

BTW without any changes to ts-command-line, we could convert -p into a string array, so you could do this:

$ rush add -p lib1 -p lib2 -p lib3 -p lib4 -p lib5

The current behavior when you supply rush add -p foo -p bar is particularly surprising - instead of erroring out since this isn't supported, it just ignores the package names before the last one supplied. You end up only adding bar. That's probably a Bad Thing™

Damn... You guys are really making me want to fix the ts-command-line backend parser, though! Today it's based on argparse which hasn't been updated in over a year...

@slavafomin
Copy link

Well, this is extremely annoying now to add multiple packages through the rush add command. And we mostly add multiple packages, one for the library itself and another one for it's typings.

It looks like this:

rush add --caret --dev -p some-package --no-update
rush add --caret --dev -p @types/some-package

which is very cumbersome.

It would be nice to do just:

rush add --caret --dev some-package @types/some-package

Also, why --dev flag doesn't have a short version -D like in npm?

I believe it would be very beneficial to have a CLI syntax compatible to npm, for once it would help with migration and adaptation.

@Js-Brecht
Copy link
Contributor

@octogonz What are your thoughts on using defineCommandLineRemainder() (#1869) to resolve this issue? Do you think it could work?

@shedali
Copy link

shedali commented Jan 16, 2021

running into this multiple times a day, it can be a bit jarring if having to switch between different package managers. Using bash function as workaround for now

function rushadd(){
  for arg in $@; do
      rush add -p $arg;
   done
}

@octogonz
Copy link
Collaborator

What are your thoughts on using defineCommandLineRemainder() (#1869) to resolve this issue? Do you think it could work?

I'm not opposed to $ rush add lib1 lib2 lib3 lib4 lib5, but it's a new syntax that may require some design discussion. (Also, as I recall after we implemented defineCommandLineRemainder() it turned out to have a parsing ambiguity, that made me want to rethink its design. I don't remember the details offhand, though.)

If we want to get something implemented quickly, it would be simpler to extend the current design like this:

$ rush add -p lib1 -p lib2 -p lib3 -p lib4 -p lib5

That would just involve changing this:

this._packageName = this.defineStringParameter({
parameterLongName: '--package',
parameterShortName: '-p',
required: true,
argumentName: 'PACKAGE',
description:
'(Required) The name of the package which should be added as a dependency.' +
' A SemVer version specifier can be appended after an "@" sign. WARNING: Symbol characters' +
" are usually interpreted by your shell, so it's recommended to use quotes." +
' For example, write "rush add --package "example@^1.2.3"" instead of "rush add --package example@^1.2.3".'
});

...to be a CommandLineStringListParameter instead of CommandLineStringParameter, and then add a for-loop somewhere to process each of the names.

Should be a pretty easy PR, and without any open questions for the design spec as far as I can tell. This would get something implemented quickly, and then we could come back and improve the CLI in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants