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] Add defaultCommand command-line parameter #4236

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

Conversation

neil585456525
Copy link

@neil585456525 neil585456525 commented Jul 13, 2023

Summary

Add defaultCommand command-line parameter to Buld and Phase command kinds.

Fixes #1168
Fixes #1797

Details

So far even with shellCommand, we still don't have a official way to override or ser a default command.

Before I found a workaround like bellow:

{
  "commandKind": "bulk",
  "name": "type:gen-all",
  "summary": "Generate type.d.ts",
  "description": "Generate type.d.ts",
  "enableParallelism": true,
  "shellCommand":"pnpm type:gen || echo no-script-to-run"
}

But it's quite tricky, accourding to that I suggest to have a defaultCommand parameter for this situation,
Comapare to shellCommand, it has a lower priority than package.json script, so user can use script field as an override, and set defaultCommands to prevent verbose set empty script string in each project which need to be skiped.
I have considered about using a flag like priority to reuse the the shellCommand, but I think it will become a mess, but it's open for discussion.

With this update, we can finally set default command like bellow:

{
  "commandKind": "bulk",
  "name": "type:gen",
  "summary": "Generate type.d.ts",
  "description": "Generate type.d.ts",
  "enableParallelism": true,
  "defaultCommand":"echo no-script-to-run"
}

How it was tested

test cases:

Impacted documentation

command-line.json

@neil585456525
Copy link
Author

@microsoft-github-policy-service agree

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

One of the motivations for "missingScriptBehavior" being an enumeration instead of adding another boolean to silence missing scripts was that a new value could be added to it for "shell-command" or similar. I don't think I'd consciously realized that the current "shellCommand" field overrides package.json.

At minimum, I think it makes more sense to add a way to control the priority of the existing "shellCommand" property than to add a second one, since it doesn't make sense to define two separate shell commands in the command definition.

@@ -294,4 +294,30 @@ describe(CommandLineConfiguration.name, () => {
expect(phase.shellCommand).toEqual('echo');
});
});

describe('defaultCommand in bulk command', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include a unit test of having it in a phase definition as well.

@neil585456525
Copy link
Author

Hi @dmichon-msft , thanks for your response.

missingScriptBehavior indeed useful for that case!

Back to the command priority. Yes, there has a restriction to let "shellCommand" field overrides package.json with a unit test to ensure this behaviar:

Agreen with the solution to have a flag to control the priority, I have some proposals:
Flag Name

  • priorityCommand
  • highestCommandPriority

Which's value so far could be shellCommand(default) or packageJsonScript, leave some flexibilty to have another kind of priority in the future.

Please feel free to give your own opinion and suggestion!

@iclanton iclanton added this to In Progress in Bug Triage Jul 24, 2023
@iclanton
Copy link
Member

Can well call it fallbackCommand or something similar to clearly show that it will only run if the command isn't implemented in the project?

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
3 participants