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.

Sign up for GitHub

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

Reduce complexity of src/cli/index.js #2887

Merged
merged 17 commits into from
Apr 10, 2017
Merged

Reduce complexity of src/cli/index.js #2887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1c3bd30
clarify initial part of src/cli/index.js when we extracting arguments…
voxsim Feb 28, 2017
123b5bf
change test if we do not provide any command name and specify a flag …
voxsim Feb 28, 2017
6ec7f97
fix silent error in test when we expect an error from a yarn command
voxsim Feb 28, 2017
f268378
enrich commands with aliases
voxsim Mar 10, 2017
c9affc7
simplify if statement: if no command we set install as default
voxsim Mar 10, 2017
04c3ce1
simplify if statement: we always have command with value undefined
voxsim Mar 10, 2017
89cb665
put every logic related to help in help command
voxsim Mar 10, 2017
0f268f3
use deconstructuring instead of concat and some shift/unshit command
voxsim Mar 10, 2017
89fdcbc
remove useless invariant on commandName: we are sure that is always d…
voxsim Mar 10, 2017
c227e40
if command is not recognized set default to run; if command is run we…
voxsim Mar 10, 2017
fbd0c88
add some tests cases when we do not recognize command
voxsim Mar 11, 2017
3314951
we use commander.js only to parse flags, remove every logic to put co…
voxsim Mar 11, 2017
0899ac7
implement hasWrapper function for help command
voxsim Feb 28, 2017
71072db
display correct help link for aliases
voxsim Mar 11, 2017
4d930e9
display documentation link correctly in every case
voxsim Mar 11, 2017
045bd60
add some console.asserts to ensure that we early crash if something i…
voxsim Apr 10, 2017
2fb8a3e
fix erronous case: yarn constructor
voxsim Apr 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 69 additions & 10 deletions __tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ Promise<Array<?string>> {
}

return new Promise((resolve, reject) => {
exec(`node "${yarnBin}" ${cmd} ${args.join(' ')}`, {cwd:workingDir, env:process.env}, (err, stdout) => {
if (err) {
reject(err);
exec(`node "${yarnBin}" ${cmd} ${args.join(' ')}`, {cwd:workingDir, env:process.env}, (error, stdout) => {
if (error) {
reject({error, stdout});
} else {
const stdoutLines = stdout.toString()
.split('\n')
Expand Down Expand Up @@ -76,13 +76,23 @@ function expectHelpOutputAsSubcommand(stdout) {
}

function expectAnErrorMessage(command: Promise<Array<?string>>, error: string) : Promise<void> {
return command.catch((reason) =>
expect(reason.message).toContain(error),
return command
.then(function() {
throw new Error('the command did not fail');
})
.catch((reason) =>
expect(reason.error.message).toContain(error),
);
}

function expectInstallOutput(stdout) {
expect(stdout[0]).toEqual(`yarn install v${pkg.version}`);
function expectAnInfoMessageAfterError(command: Promise<Array<?string>>, info: string) : Promise<void> {
return command
.then(function() {
throw new Error('the command did not fail');
})
.catch((reason) =>
expect(reason.stdout).toContain(info),
);
}

test.concurrent('should add package', async () => {
Expand Down Expand Up @@ -182,12 +192,12 @@ test.concurrent('should run --version command', async () => {

test.concurrent('should install if no args', async () => {
const stdout = await execCommand('', [], 'run-add', true);
expectInstallOutput(stdout);
expect(stdout[0]).toEqual(`yarn install v${pkg.version}`);
});

test.concurrent('should install if first arg looks like a flag', async () => {
const stdout = await execCommand('--offline', [], 'run-add', true);
expectInstallOutput(stdout);
const stdout = await execCommand('--json', [], 'run-add', true);
expect(stdout[stdout.length - 1]).toEqual('{"type":"success","data":"Saved lockfile."}');
});

test.concurrent('should interpolate aliases', async () => {
Expand All @@ -197,6 +207,13 @@ test.concurrent('should interpolate aliases', async () => {
);
});

test.concurrent('should display correct documentation link for aliases', async () => {
await expectAnInfoMessageAfterError(
execCommand('i', [], 'run-add', true),
'Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.',
);
});

test.concurrent('should run help of run command if --help is before --', async () => {
const stdout = await execCommand('run', ['custom-script', '--help', '--'], 'run-custom-script-with-arguments');
expect(stdout[0]).toEqual('Usage: yarn [command] [flags]');
Expand All @@ -216,3 +233,45 @@ test.concurrent('should run bin command', async () => {
expect(stdout[0]).toEqual(path.join(fixturesLoc, 'node_modules', '.bin'));
expect(stdout.length).toEqual(1);
});

test.concurrent('should throws missing command for not camelised command', async () => {
await expectAnErrorMessage(
execCommand('HelP', [], 'run-add', true),
'Command \"HelP\" not found',
);
});

test.concurrent('should throws missing command for not alphabetic command', async () => {
await expectAnErrorMessage(
execCommand('123', [], 'run-add', true),
'Command \"123\" not found',
);
});

test.concurrent('should throws missing command for unknown command', async () => {
await expectAnErrorMessage(
execCommand('unknown', [], 'run-add', true),
'Command \"unknown\" not found',
);
});

test.concurrent('should not display documentation link for unknown command', async () => {
await expectAnInfoMessageAfterError(
execCommand('unknown', [], 'run-add', true),
'',
);
});

test.concurrent('should display documentation link for known command', async () => {
await expectAnInfoMessageAfterError(
execCommand('add', [], 'run-add', true),
'Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.',
);
});

test.concurrent('should throws missing command for constructor command', async () => {
await expectAnErrorMessage(
execCommand('constructor', [], 'run-add', true),
'Command \"constructor\" not found',
);
});
3 changes: 3 additions & 0 deletions __tests__/lifecycle-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ async () => {
stdout = await execCommand('', 'npm_config_argv_env_vars', env);
expect(stdout).toContain('##install##');

stdout = await execCommand('run test', 'npm_config_argv_env_vars', env);
expect(stdout).toContain('##test##');

stdout = await execCommand('test', 'npm_config_argv_env_vars', env);
expect(stdout).toContain('##test##');
});
Expand Down
57 changes: 42 additions & 15 deletions src/cli/commands/help.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@ import * as commands from './index.js';
import * as constants from '../../constants.js';
import type {Reporter} from '../../reporters/index.js';
import type Config from '../../config.js';
import {sortAlpha, hyphenate} from '../../util/misc.js';
import {sortAlpha, hyphenate, camelCase} from '../../util/misc.js';
const chalk = require('chalk');

export function hasWrapper(): boolean {
return false;
}

export function run(
config: Config,
reporter: Reporter,
Expand All @@ -17,24 +21,47 @@ export function run(
const getDocsInfo = (name) => 'Visit ' + chalk.bold(getDocsLink(name)) + ' for documentation about this command.';

if (args.length) {
const helpCommand = hyphenate(args[0]);
if (commands[helpCommand]) {
commander.on('--help', () => console.log(' ' + getDocsInfo(helpCommand) + '\n'));
}
} else {
commander.on('--help', () => {
console.log(' Commands:\n');
for (const name of Object.keys(commands).sort(sortAlpha)) {
if (commands[name].useless) {
continue;
const commandName = camelCase(args.shift());

if (commandName) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check what happened when i type yarn unknown-command --help. It should ignore everything and display standard help. Maybe this behaviour is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I was right. In detail: yarn unknown --help

before:

  Usage: yarn [command] [flags]

  Options:

    -h, --help                      output usage information
    -V, --version                   output the version number
    --verbose                       output verbose messages on internal operations
    --offline                       trigger an error if any required dependencies are not available in local cache
    --prefer-offline                use network only if dependencies are not available in local cache
    --strict-semver
    --json
    --ignore-scripts                don't run lifecycle scripts
    --har                           save HAR output of network traffic
    --ignore-platform               ignore platform checks
    --ignore-engines                ignore engines check
    --ignore-optional               ignore optional dependencies
    --force                         ignore all caches
    --no-bin-links                  don't generate bin links when setting up packages
    --flat                          only allow one version of a package
    --prod, --production [prod]
    --no-lockfile                   don't read or generate a lockfile
    --pure-lockfile                 don't generate a lockfile
    --frozen-lockfile               don't generate a lockfile and fail if an update is needed
    --link-duplicates               create hardlinks to the repeated modules in node_modules
    --global-folder <path>
    --modules-folder <path>         rather than installing modules into the node_modules folder relative to the cwd, output them here
    --cache-folder <path>           specify a custom folder to store the yarn cache
    --mutex <type>[:specifier]      use a mutex to ensure only one yarn instance is executing
    --no-emoji                      disable emoji in output
    --proxy <host>
    --https-proxy <host>
    --no-progress                   disable progress bar
    --network-concurrency <number>  maximum number of concurrent network requests

after pr:

    Usage: yarn [command] [flags]

  Options:

    -h, --help                      output usage information
    -V, --version                   output the version number
    --verbose                       output verbose messages on internal operations
    --offline                       trigger an error if any required dependencies are not available in local cache
    --prefer-offline                use network only if dependencies are not available in local cache
    --strict-semver
    --json
    --ignore-scripts                don't run lifecycle scripts
    --har                           save HAR output of network traffic
    --ignore-platform               ignore platform checks
    --ignore-engines                ignore engines check
    --ignore-optional               ignore optional dependencies
    --force                         install and build packages even if they were built before, overwrite lockfile
    --skip-integrity-check          run install without checking if node_modules is installed
    --no-bin-links                  don't generate bin links when setting up packages
    --flat                          only allow one version of a package
    --prod, --production [prod]
    --no-lockfile                   don't read or generate a lockfile
    --pure-lockfile                 don't generate a lockfile
    --frozen-lockfile               don't generate a lockfile and fail if an update is needed
    --link-duplicates               create hardlinks to the repeated modules in node_modules
    --global-folder <path>
    --modules-folder <path>         rather than installing modules into the node_modules folder relative to the cwd, output them here
    --cache-folder <path>           specify a custom folder to store the yarn cache
    --mutex <type>[:specifier]      use a mutex to ensure only one yarn instance is executing
    --no-emoji                      disable emoji in output
    --proxy <host>
    --https-proxy <host>
    --no-progress                   disable progress bar
    --network-concurrency <number>  maximum number of concurrent network requests
    --non-interactive               do not show interactive prompts

  Commands:

    - access
    - add
    - bin
    - cache
    - check
    - clean
    - config
    - generate-lock-entry
    - global
    - help
    - import
    - info
    - init
    - install
    - licenses
    - link
    - list
    - login
    - logout
    - outdated
    - owner
    - pack
    - publish
    - remove
    - run
    - tag
    - team
    - unlink
    - upgrade
    - upgrade-interactive
    - version
    - versions
    - why

  Run `yarn help COMMAND` for more information on specific commands.
  Visit https://yarnpkg.com/en/docs/cli/ to learn more about Yarn.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea! I'd probably put them on a single line, comma-separate, restricted to 80 columns. Otherwise they take a lot of vertical space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the commands part? I can do that if you confirm

Copy link
Member

Choose a reason for hiding this comment

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

Yep!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't overwrite the commands part without doing other horrible things for what I understand because it is a feature of commander.js -.-. In detail the function help.

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think it's done userland, more precisely here. That being said, I've checked locally and it seems that this display (one command each line) was already printed this way in recent Yarn releases, so let's keep it this way. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! As you wish :)

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to check this, since it won't pass the following test (commands[commandName] will be undefined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to check this because commandName can be null, sorry I lost this comment!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah ... it seems this camelCase function should rather be called ifCamelCaseNullElseCamelCase 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YES T_T XD

const command = commands[commandName];
Copy link
Member

Choose a reason for hiding this comment

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

We probably should validate every such access with Object.prototype.hasOwnProperty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we validate here maybe we should validate in src/cli/index.js. What do you think?

Copy link
Member

@arcanis arcanis Apr 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yep! It will fix this:

❯ [mael-mbp?] yarn git:(0.23-stable) ❯ yarn constructor

yarn constructor v0.23.0
error An unexpected error occurred: "command.run is not a function".
info If you think this is a bug, please open a bug report with the information provided in "/Users/mael/yarn/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/constructor for documentation about this command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pre-existent error, but i prefer to fix here right now. If I am correctly after this part, we will finish this review.

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arcanis done!


if (command) {
if (typeof command.setFlags === 'function') {
command.setFlags(commander);
}

const examples: Array<string> = (command && command.examples) || [];
if (examples.length) {
commander.on('--help', () => {
console.log(' Examples:\n');
for (const example of examples) {
console.log(` $ yarn ${example}`);
}
console.log();
});
}
commander.on('--help', () => console.log(' ' + getDocsInfo(commandName) + '\n'));

console.log(` - ${hyphenate(name)}`);
commander.help();
return Promise.resolve();
}
console.log('\n Run `' + chalk.bold('yarn help COMMAND') + '` for more information on specific commands.');
console.log(' Visit ' + chalk.bold(getDocsLink()) + ' to learn more about Yarn.\n');
});
}
}

commander.on('--help', () => {
console.log(' Commands:\n');
for (const name of Object.keys(commands).sort(sortAlpha)) {
if (commands[name].useless) {
continue;
}

console.log(` - ${hyphenate(name)}`);
}
console.log('\n Run `' + chalk.bold('yarn help COMMAND') + '` for more information on specific commands.');
console.log(' Visit ' + chalk.bold(getDocsLink()) + ' to learn more about Yarn.\n');
});

commander.help();
return Promise.resolve();
}
Loading