Repository: pypa/pip
Pull request: Show appropriate error message
This catches code exception if wrong arguments passed to cmd options
It also adds error message that suggests correct arguments to pass

Fixes: https://github.com/pypa/pip/issues/5616

<!---
Thank you for your soon to be pull request. Before you submit this, please
double check to make sure that you've added a news file fragment. In pip we
generate our NEWS.rst from multiple news fragment files, and all pull requests
require either a news file fragment or a marker to indicate they don't require
one.

To read more about adding a news file fragment for your PR, please check out
our documentation at: https://pip.pypa.io/en/latest/development/#adding-a-news-entry
-->


Problem statement:
Invalid values in boolean options in the environment cause traceback
**Environment**

* pip version: 10.0.1
* Python version: 3.6.6
* OS: Debian Unstable

**Description**

When setting an environment variable that is supposed to hold a boolean value to an invalid string that is not empty, a traceback happens.

**Expected behavior**

An error message along the lines of "'blah' is not a valid value for ..., valid values

**How to Reproduce**

1. `pip install pip==10.0.1`
2. Run `env PIP_REQUIRE_VIRTUALENV='blah' PYTHONPATH=src python -m pip install twisted`
3. Observe a traceback

**Output**

```
$ env PIP_REQUIRE_VIRTUALENV='blah' PYTHONPATH=src python -m pip install twisted                                                       
Traceback (most recent call last):                                             
  File "/home/moshez/.pyenv/versions/3.6.4/lib/python3.6/runpy.py", line 193, in
 _run_module_as_main                                                           
...
    values = self.get_default_values()
  File "/home/moshez/src/pip/src/pip/_internal/baseparser.py", line 230, in get_default_values
    defaults = self._update_defaults(self.defaults.copy())  # ours             
  File "/home/moshez/src/pip/src/pip/_internal/baseparser.py", line 195, in _update_defaults
    val = strtobool(val)
  File "/home/moshez/.pyenv/versions/3.6.4/lib/python3.6/distutils/util.py", line 317, in strtobool
    raise ValueError("invalid truth value %r" % (val,))                        
ValueError: invalid truth value 'blah'
```

*Note*

The documentation of `strtobool` (seen in the traceback above) says:

"True values are y, yes, t, true, on and 1; false values are n, no, f, false, off and 0. Raises ValueError if val is anything else."

Hints from issue discussion:
Definitely a place where we can print a better error message. :)
This issue is a good starting point for anyone who wants to help out with pip's development -- it's simple and the process of fixing this should be a good introduction to pip's development workflow. See the discussion above to understand what the desired fix is.

Feel free to mention me (write @pradyunsg in the comment) if you need help. :)

@pradyunsg can I take this up?
Sure. :)
@sinscary How's it going? Did you get some time to look into this?
@pradyunsg Sorry, actually I was kinda busy. Will try to finish it by today.
@pradyunsg what will be an appropriate error message in this case. What I can think of is - 
"`blah` is not a valid value for require-virtualenv".
Yeah. And then have it use the default value that it would.

store_true's default is False, counts default is 0 and so on.
@pradyunsg So shouldn't it be exiting the process if `ValueError` occurs? 
What I understood from your suggestion is, just print the error message and then assign default values to `store_true`, `store_false` and `count` and move on with the process.
Yeah... That's what I was suggesting.

Actually what you're suggesting also makes sense; pip should error out if the configuration provided to it is incorrect.

Commit message: Show appropriate error message

This catches code exception if wrong arguments passed to cmd options
It also adds error message that suggests correct arguments to pass

Fixes: https://github.com/pypa/pip/issues/5616

Patch to review:
diff --git a/news/5644.bugfix b/news/5644.bugfix
new file mode 100644
index 00000000000..8cbbcaf13c1
--- /dev/null
+++ b/news/5644.bugfix
@@ -0,0 +1 @@
+Show appropriate error message if invalid args passed
\ No newline at end of file
diff --git a/src/pip/_internal/baseparser.py b/src/pip/_internal/baseparser.py
index 9a8d129747e..68a29655f5c 100644
--- a/src/pip/_internal/baseparser.py
+++ b/src/pip/_internal/baseparser.py
@@ -192,7 +192,17 @@ def _update_defaults(self, defaults):
                 continue
 
             if option.action in ('store_true', 'store_false', 'count'):
-                val = strtobool(val)
+                try:
+                    val = strtobool(val)
+                except ValueError:
+                    self.error(
+                        "#{0} is not a valid value. Valid true values"
+                        "are {1}, {2}, {3}, {4} and valid false values"
+                        "are {5}, {6}, {7}, {8}".format(
+                            val, "true", "yes", "on", 1,
+                            "false", "no", "off", 0
+                        )
+                    )
             elif option.action == 'append':
                 val = val.split()
                 val = [self.check_default(option, key, v) for v in val]


Code review comments:
diff_hunk: @@ -192,7 +192,17 @@ def _update_defaults(self, defaults):
                 continue
 
             if option.action in ('store_true', 'store_false', 'count'):
-                val = strtobool(val)
+                try:
+                    val = strtobool(val)
+                except ValueError:
+                    self.error(
+                        "#{0} is not a valid value. Valid true values"
path: src/pip/_internal/baseparser.py
text: @user1:
This doesn't consider the case of `count` actions. Also, you don't need to format the constant values into the string. :)

It would be nice to have error messages like:

```
'blah' is not a valid value for require-virtualenv option, please specify a truth value like yes/no, true/false or 1/0 instead.
```

```
'blah' is not a valid value for <some count option> option, please specify a numerical value instead.
```

@author:
@user1 as I went through the codebase, I realized that this option is not only for `require-virtualenv`. `store_true`, `store_false`, `count` all three are being used for multiple options. So if this error has to be action specific than it would be better to have a separate function which will decide the error message based on `option.action` which error to return. something like this - 

```
def fun(action, key):
    if action == 'count':
         return "count related error message"
    elif action == 'store_false':
         return "store_false related error message"
    elif action == 'store_true':
         return "store_true related error message"
```

what say?


@user1:
I was just using `require-virtualenv` as an example. If you can substitute the name of the option being used, that would be nice, since it would provide more hint to the user what caused the error.

Having a function to get the correct message, sounds like a good idea. :)