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] command-line.json supports specifying custom remainder for commands and phases #3776

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chengcyber
Copy link
Contributor

@chengcyber chengcyber commented Nov 22, 2022

Summary

This PR proposes specifying custom remainder for commands and phases in "command-line.json"

Details

  • Add "remainders" property in "command-line.json"
  • Each item in "remainders" associates with commands or phases
  • The associated commands/phases can get the argument list after recognized portion.

How it was tested

modify "common/config/rush/command-line.json"

  "commands": [
    {
      "commandKind": "global",
      "name": "echo",
      "summary": "echo argv",
      "shellCommand": "echo $@"
    },

   // ...

   "remainders": [
     {
        "description": "rest arguments"
        "associatedCommands": ["echo"]
     }
   ]

Run node libraries/rush-lib/lib/start.js echo a b c, it prints

image

Run node libraries/rush-lib/lib/start.js echo -h, it prints

image

@iclanton iclanton added this to In Progress in Bug Triage Dec 5, 2022
*/
associatedCommands?: string[];
/**
* A list of the names of the phases that this command-line parameter should be provided to.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A list of the names of the phases that this command-line parameter should be provided to.
* A list of the names of the phases that the remainder of the command-line should be provided to.

*/
description: string;
/**
* A list of custom commands and/or built-in Rush commands that this parameter may be used with, by name.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A list of custom commands and/or built-in Rush commands that this parameter may be used with, by name.
* A list of custom commands and/or built-in Rush commands the remainder of the command-line may be used with, by name.

@@ -271,4 +290,5 @@ export interface ICommandLineJson {
commands?: CommandJson[];
phases?: IPhaseJson[];
parameters?: ParameterJson[];
remainders?: IRemainderJson[];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
remainders?: IRemainderJson[];
commandLineRemainers?: IRemainderJson[];

*/
export interface IRemainderJson {
/**
* Denotes that this is a remainder parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Denotes that this is a remainder parameter.
* The description of how the command-line remainder for the specified commands may be used.

Comment on lines +45 to +47
protected get customRemainderJson(): IRemainderJson | undefined {
return this._customRemainderJson;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this useful to have as part of this class's API?

"additionalProperties": false,
"required": ["description"],
"properties": {
"description": { "$ref": "#/definitions/anything" },
Copy link
Member

Choose a reason for hiding this comment

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

This should be a string, right?

Comment on lines +673 to +674
"associatedCommands": { "$ref": "#/definitions/anything" },
"associatedPhases": { "$ref": "#/definitions/anything" }
Copy link
Member

Choose a reason for hiding this comment

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

These should be string arrays, right?

@iclanton
Copy link
Member

iclanton commented Dec 6, 2022

What happens when a command/phase is associated with two remainders?

My inclination would be to treat the remainder as a singleton that can be associated with commands or phases, and just have a hardcoded description.

@chengcyber
Copy link
Contributor Author

chengcyber commented Dec 7, 2022

What happens when a command/phase is associated with two remainders?

Yes. remainder should be a singleton. We should forbid (throw error) when declaring two remainders for the same command

My inclination would be to treat the remainder as a singleton that can be associated with commands or phases, and just have a hardcoded description.

Are you proposing something like the following?

common/config/rush/command-line.json

{
   "remainders": {  // a list of different remainders -> a object declares associated commands and/or phases
      // no description setting at all
      "associatedCommands": [ "FOO", "BAR" ]         // should be unique
      "associatedPhases": ["_phase:FOO", "_phase:BAR"]      //  should be unique
   }
}

If so, the only concern is there is no way to archive the use case as follow:

   "remainders": [
      {
         "associatedCommands": [ "FOO"]         
        "associatedPhases": ["_phase:FOO", "_phase:BAR"]    
      },
      {
         "associatedCommands": ["BAR" ]         
        "associatedPhases": ["_phase:BAR"]    
      }
   ]

custom remainder for _phase:FOO only for FOO command, but not for BAR command.

Maybe the use case is not a big deal (?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Bug Triage
  
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants