Pip: Replace generic (*args, **kwargs) with specific parameters in pip._internal.command classes

[deveshks]

As per the comment in https://github.com/pypa/pip/pull/8051#discussion_r425724481 , we are using generic *args, **kwargs at a lot of places where we know only specific set of parameters are being called.

For ex.

https://github.com/pypa/pip/blob/7e44950e72e891c9096e095f817385a6f8166e29/src/pip/_internal/commands/debug.py#L196-L198

and https://github.com/pypa/pip/blob/7e44950e72e891c9096e095f817385a6f8166e29/src/pip/_internal/commands/debug.py#L196-L198

We would want to create a PR that removes these and replaces them with specific params

[gutsytechster]

@deveshks let me know if you are working over this. Else I might want to give it a try! :)

[McSinyx]

@gutsytechster, I made some quick search, hopefully you find it helpful:

$ ack create_command src
src/pip/_internal/commands/help.py
20:            commands_dict, create_command, get_similar_commands,
38:        command = create_command(cmd_name)

src/pip/_internal/commands/__init__.py
98:def create_command(name, **kwargs):

src/pip/_internal/cli/main.py
12:from pip._internal.commands import create_command
73:    command = create_command(cmd_name, isolated=("--isolated" in cmd_args))

src/pip/_internal/cli/autocompletion.py
10:from pip._internal.commands import commands_dict, create_command
64:        subcommand = create_command(subcommand_name)

In commands:

def create_command(name, **kwargs):
    # type: (str, **Any) -> Command
    """
    Create an instance of the Command class with the given name.
    """
    module_path, class_name, summary = commands_dict[name]
    module = importlib.import_module(module_path)
    command_class = getattr(module, class_name)
    command = command_class(name=name, summary=summary, **kwargs)

    return command

[deveshks]

I have a few PRs open to add remaining type annotations in some of the files of pip._internal.commands, (https://github.com/pypa/pip/pull/8016 and https://github.com/pypa/pip/pull/8018)

I think this will be much easier to tackle this in those specific files once those PRs are merged in, but you can take a shot at some of the other files not used in those PRs and present in https://github.com/pypa/pip/tree/master/src/pip/_internal/commands to attempt this.

You can add missing type annotations along with adding this in those files as well.

[gutsytechster]

@deveshks Does that mean, like changing this

    def __init__(self, *args, **kw):
        # type: (*Any, **Any) -> None
        super(DebugCommand, self).__init__(*args, **kw)

        cmd_opts = self.cmd_opts

to this?

   def __init__(self, cmd_opts):
        # type: (*Any, **Any) -> None
        super(DebugCommand, self).__init__(cmd_opts)

        cmd_opts = self.cmd_opts

[McSinyx]

I think __init__ should have the signature of (name, summary, isolated) (with type and default values especially for isolated).

[deveshks]

I think __init__ should have the signature of (name, summary, isolated) (with type and default values especially for isolated).

Yep, something like this for debug.py for example.

diff --git a/src/pip/_internal/commands/debug.py b/src/pip/_internal/commands/debug.py
index 8e243011..1d1485f8 100644
--- a/src/pip/_internal/commands/debug.py
+++ b/src/pip/_internal/commands/debug.py
@@ -193,9 +193,9 @@ class DebugCommand(Command):
       %prog <options>"""
     ignore_require_venv = True

-    def __init__(self, *args, **kw):
-        # type: (*Any, **Any) -> None
-        super(DebugCommand, self).__init__(*args, **kw)
+    def __init__(self, name, summary, isolated=False):
+        # type: (str, str, bool) -> None
+        super(DebugCommand, self).__init__(name, summary, isolated=isolated)

         cmd_opts = self.cmd_opts
         cmdoptions.add_target_python_options(cmd_opts)

Also aim at only modifying a few files at a time per PR.

[pradyunsg]

Hmm... I think a much cleaner approach would be to create an additional add_options method, that these classes use. Then, Command.__init__ can be modified to call this self.add_options, with the default implementation doing nothing.

diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py
index c9b9ea4a..a5293ae3 100644
--- a/src/pip/_internal/commands/install.py
+++ b/src/pip/_internal/commands/install.py
@@ -87,11 +87,7 @@ class InstallCommand(RequirementCommand):
       %prog [options] [-e] <local project path> ...
       %prog [options] <archive url/path> ..."""

-    def __init__(self, *args, **kw):
-        super(InstallCommand, self).__init__(*args, **kw)
-
-        cmd_opts = self.cmd_opts
-
+    def add_options(self, cmd_opts):
+        # type: (optparse.OptionGroup) -> None
         cmd_opts.add_option(cmdoptions.requirements())
         cmd_opts.add_option(cmdoptions.constraints())
         cmd_opts.add_option(cmdoptions.no_deps())

That's less redundancy/boilerplate in each command definition, and more clearly indicates the intent. :)

[gutsytechster]

@pradyunsg that's great!

But I am confused as to how can we keep the PR small as changes in base_command.Command would affect all other commands as they are inheriting it?

[pradyunsg]

We can't keep it smaller than that, and that's OK, as long as we're not changing anything else in the PR.

The exercise of making smaller PRs is mainly for making it easier to review those changes, to avoid conflicts and to minimize what would need to be reverted (in case a regression is introduced). Generally, the optimal size for PRs would be "least complex self-contained change". The complexity of making that change is a mix of practical complexity (changing too many lines, or making changes to a constantly changing part of the codebase like tests, stuff that can cause merge conflicts, inconsistent logic etc) as well as conceptual complexity (affecting multiple "things" - updating docs for feature X and updating tests for feature Y should be separate).

I think updating all the commands to this updated signature (and nothing else!) is pretty self contained change, that's doing only one "thing" and isn't affecting an area with lot of moving parts that would result in conflicts.

(the explanation above could probably be polished off a bit, with some restructuring, but it's fine as it is)

[gutsytechster]

I also notice that commands like cache, check and help do not add other cmd_opts. In that case, how should the definition for the suggested add_option method go? I mean if we don't define any add_option then that would raise the error as we would be calling it within the __init__ of the base class.

Does defining the method and including only pass statement make sense? Or we can only call the method if it is available in the child class.