Prompt before listing all repos when running bower search without a query param

[eppeters]

Description

This PR aims to address issue #2066 by warning the user before attempting to list all packages when using bower search with no query parameter.

In interactive mode

When running bower search with no additional params, the user should see the following:

? Running search with no search term will list all bower packages and may take a while to complete. Continue? (Y/n)

If they confirm the action, the search will be performed. If they deny the action, No results. will be printed and they will be returned to their prompt.

In non-interactive (programmatic mode)

The search will just be performed without any prompt.

Tests

The PR includes tests that no prompt occurs in non-interactive mode (no prompt, just like it was before the PR), and tests that the full listing is generated in interactive mode if and only if the user agrees to continue past the warning.

Note

This is my first PR to an open source project, and therefore my first to bower as well, so let me know if I'm doing something incorrectly.

[sheerun]

Hey, thank you 😊

1. Could you squash all the commits
2. Could we just show "help" screen instead of prompt for interactive mode? I think it's never the case users want to see all the packages in interactive mode..

[sheerun]

Could we also leave it as it is when config.json is enabled?

[eppeters]

@sheerun, I'm willing to add in the help screen in this case, but I just noticed the current behavior for the other commands that "require" an argument, such as bower unregister and bower info, is to simply exit without a message.

Wouldn't it be better to create a seperate, more general issue to add an easy way to default to the help screen inside a command, and then we could make the behavior of all of these commands consistent. It would be odd to just do one at a time, with potentially different solutions for showing the help screen from inside the command.

[sheerun]

Yes, it would be odd :) Could you open another issues about it?

But for now let's implement it at least for search command, especially the behavior is changing.

[eppeters]

Will do.

[heinst]

I guess people don't respect the asignees in Github... I found this bug and assigned myself the issue and was working on it in my spare time, oh well time to scrap my work

[sheerun]

I'm sorry to hear it :( It's probably better to write a comment. Often people assign issues on them by mistake. Or assign, abandon work, and forget to unassign.

[eppeters]

@heinst I apologize. This is my first contribution to an open source project, and my first time using Github (I'm familiar with Stash/Jira, but not Github for issues/PRs). I hadn't seen an assignee field at all, but going back now, I see your self-assignment. This was my mistake for sure.

I will agree that it is harder to see the assignment field/notification than I would've expected, though that's not your fault.

[eppeters]

To give an update here, I'm having some trouble calling the help display when command argument parsing fails. This is because we don't have an official way to deal with command argument parsing, as well as some bad coupling between the logger and commands.

There's this from bin/bower:

    // Call the line method
    } else {
        logger = commandFunc.line(process.argv);

        // If the method failed to interpret the process arguments
        // show the command help
        if (!logger) {
            logger = bower.commands.help(command);
            command = 'help';
        }
    }

Note the // If the method failed to interpret..., which indicates that bower expects no logger when argument parsing fails, and will default to the help display. So the intention seemed to be the functionality requested here for the bower search command.

However, we always return a valid logger, so there is no way for bin/bower to fallback to the help command.

From lib/commands/index, commandFactory()

    function runFromArgv(argv) {
        return withLogger(function (logger) {
            var command = require(id);

            var commandArgs = command.readOptions(argv);
            commandArgs.unshift(logger);

            return command.apply(undefined, commandArgs);
        });
    }

    function withLogger(func) {
        var logger = new Logger();

        Q.try(func, logger)
        .done(function () {
            config.restore();
            var args = [].slice.call(arguments);
            args.unshift('end');
            logger.emit.apply(logger, args);
        }, function (error) {
            config.restore();
            logger.emit('error', error);
        });

        return logger;
    }

    command.line = runFromArgv;

So command.line() runs runFromArgv(), which in turn returns the value of withLogger(), which is always a logger. The logger then asynchronously gets injected into the command, the command is run, and listeners are set up in bin/bower to render the result of logging. If we returned a falsy value instead of the logger when the command couldn't parse the args, bin/bower would default to showing the output of bower help <command>.

My plan is to at least decouple the argument checking from the command being run, so that we can check the params synchronously (using command.readOptions()), and synchronously return null instead of a Logger when the params are invalid. This will automatically make bower default to the help command if we hook each command's readOptions() (in this case, just bower search) to error when the params are invalid.

The only problem I see is that other places using the lib/commands/index interface may always be expecting a valid Logger instance to be returned from the commandFactory(), so I'll have to update tests and usages to match this change.

[sheerun]

Let's make changes as small as possible. Fortunately we have integration tests for most of the commands.

[eppeters]

@sheerun I ended up just making it possible for commands to exit by throwing an exception whenever there is a syntax error (i.e. missing parameter) on the CLI, and the bower script then defaults to bower help <command> as was originally intended. Making other commands exit and show their help during syntax error situations should be easier in the future.

I did not have to change anything to get the other commands' integration tests to pass locally.

I had to rewrite the tests for the search command though, and it looks I missed testing a new exception in search.readOptions().

[sheerun]

@eppeters Looks good to me, so I'm accepting this PR, great job :) Are you on our Discord chat?

[eppeters]

@sheerun I am now!

[sheerun]

It turns out you show help message even if someone passes --json flag..

[eppeters]

Yeah I see that now. It's just showing the json version of whatever command
gets run. If search fails because of params, the help command is run
instead. search . readOptions should not fail if --json is passed. Then the
search command will be run and logged as intended. Of course tests need to
be fixed as well.

[sheerun]

I'm working on a fix

[eppeters]

Sounds good :) Sorry for the trouble.

[eppeters]

While I was waiting for your response, I made these changes, but I understand fully if you want to go ahead and fix it.