Repository: repobee/repobee

Issue:
CLI help sections cleanup
The CLI `-h` sections are a bit bloated with stuff that you rarely put on the command line, such as the base url and the API token. It would possibly be a good idea to hide these if there are defaults, and add little note that they are hidden.

It would be even better if it was possible to specify a pre-parser option for showing them anyway.

This can also be a 2.0 feature as it's a bit tricky to implement and not crucial to current operation.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=h1) Report
> Merging [#283](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/e9ad6a984900c9ea298b080d8834a1d969cb6a03?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee/pull/283/graphs/tree.svg?width=650&token=5vOKgkphZe&height=150&src=pr)](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
- Coverage   89.86%   89.83%   -0.03%     
==========================================
  Files          19       19              
  Lines        1411     1427      +16     
  Branches      256      256              
==========================================
+ Hits         1268     1282      +14     
- Misses        120      123       +3     
+ Partials       23       22       -1
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/283/diff?src=pr&el=tree#diff-cmVwb2JlZS9jbGkucHk=) | `97.8% <100%> (-0.64%)` | :arrow_down: |
| [repobee/main.py](https://codecov.io/gh/repobee/repobee/pull/283/diff?src=pr&el=tree#diff-cmVwb2JlZS9tYWluLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [repobee/ext/gitlab.py](https://codecov.io/gh/repobee/repobee/pull/283/diff?src=pr&el=tree#diff-cmVwb2JlZS9leHQvZ2l0bGFiLnB5) | `64.59% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=footer). Last update [e9ad6a9...3052e1b](https://codecov.io/gh/repobee/repobee/pull/283?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/repobee/cli.py b/repobee/cli.py
index 7ff3eae..a775d90 100644
--- a/repobee/cli.py
+++ b/repobee/cli.py
@@ -112,9 +112,17 @@ DEPRECATED_PARSERS = {
     ),
 }
 
+# any pre-parser options go here
+PRE_PARSER_PLUG_OPTS = ["-p", "--plug"]
+PRE_PARSER_NO_PLUGS = "--no-plugins"
+PRE_PARSER_SHOW_ALL_OPTS = "--show-all-opts"
+
+# this list should include all pre-parser flags
+PRE_PARSER_FLAGS = [PRE_PARSER_NO_PLUGS, PRE_PARSER_SHOW_ALL_OPTS]
+
 
 def parse_args(
-    sys_args: Iterable[str]
+    sys_args: Iterable[str], show_all_opts=False
 ) -> (tuples.Args, Optional[apimeta.API]):
     """Parse the command line arguments and initialize an API.
 
@@ -125,7 +133,7 @@ def parse_args(
         a tuples.Args namedtuple with the arguments, and an initialized
         apimeta.API instance (or None of testing connection).
     """
-    parser = _create_parser()
+    parser = _create_parser(show_all_opts)
     args = parser.parse_args(_handle_deprecation(sys_args))
 
     if getattr(args, SUB) == SHOW_CONFIG_PARSER:
@@ -555,7 +563,7 @@ class _OrderedFormatter(argparse.HelpFormatter):
         super().add_arguments(actions)
 
 
-def _create_parser():
+def _create_parser(show_all_opts):
     """Create the parser."""
 
     parser = argparse.ArgumentParser(
@@ -574,19 +582,19 @@ def _create_parser():
         action="version",
         version="{} v{}".format(repobee.__package__, repobee.__version__),
     )
-    _add_subparsers(parser)
+    _add_subparsers(parser, show_all_opts)
     return parser
 
 
-def _add_subparsers(parser):
+def _add_subparsers(parser, show_all_opts):
     """Add all of the subparsers to the parser. Note that the parsers prefixed
     with `base_` do not have any parent parsers, so any parser inheriting from
     them must also inherit from the required `base_parser` (unless it is a
     `base_` prefixed parser, of course).
     """
 
-    base_parser, base_student_parser, master_org_parser = (
-        _create_base_parsers()
+    base_parser, base_student_parser, master_org_parser = _create_base_parsers(
+        show_all_opts
     )
 
     repo_name_parser = argparse.ArgumentParser(add_help=False)
@@ -701,11 +709,20 @@ def _add_subparsers(parser):
     )
 
 
-def _create_base_parsers():
+def _create_base_parsers(show_all_opts):
     """Create the base parsers."""
     configured_defaults = config.get_configured_defaults()
     config.execute_config_hooks()
 
+    if not show_all_opts and configured_defaults:
+        LOGGER.info(
+            "CLI options that are set in the config file are suppressed in "
+            "help sections, run with pre-parser option {all_opts_arg} to "
+            "unsuppress. Example: gitlab {all_opts_arg} setup -h".format(
+                all_opts_arg=PRE_PARSER_SHOW_ALL_OPTS
+            )
+        )
+
     def default(arg_name):
         return (
             configured_defaults[arg_name]
@@ -719,11 +736,67 @@ def _create_base_parsers():
     def api_requires(arg_name):
         return arg_name in plug.manager.hook.api_init_requires()
 
+    def hide_api_arg(arg_name):
+        # note that API args that are not required should not be shown even
+        # when show_all_opts is True, as they are not relevant.
+        return not api_requires(arg_name) or (
+            not show_all_opts and configured(arg_name)
+        )
+
+    def hide_configurable_arg(arg_name):
+        return not show_all_opts and configured(arg_name)
+
+    # API args help sections
+    user_help = argparse.SUPPRESS if hide_api_arg("user") else "Your username."
+    org_name_help = (
+        argparse.SUPPRESS
+        if hide_api_arg("org_name")
+        else "Name of the target organization"
+    )
+    base_url_help = (
+        argparse.SUPPRESS
+        if hide_api_arg("base_url")
+        else (
+            "Base url to a platform API. Must be HTTPS. For example, with "
+            "github.com, the base url is https://api.github.com, and with "
+            "GitHub enterprise, the url is https://<ENTERPRISE_HOST>/api/v3"
+        )
+    )
+    token_help = (
+        argparse.SUPPRESS
+        if hide_api_arg("token")
+        else (
+            "OAUTH token for the platform instance. Can also be specified in "
+            "the `REPOBEE_OAUTH` environment variable."
+        )
+    )
+
+    # other configurable args help sections
+    # these should not be checked against the api_requires function
+    students_file_help = (
+        argparse.SUPPRESS
+        if hide_configurable_arg("students_file")
+        else (
+            "Path to a list of student usernames. Put multiple usernames on "
+            "each line to form groups."
+        )
+    )
+    master_org_help = (
+        argparse.SUPPRESS
+        if hide_configurable_arg("master_org_name")
+        else (
+            "Name of the organization containing the master repos. "
+            "Defaults to the same value as `-o|--org-name` if left "
+            "unspecified. Note that config values take precedence "
+            "over this default."
+        )
+    )
+
     base_parser = argparse.ArgumentParser(add_help=False)
     base_parser.add_argument(
         "-u",
         "--user",
-        help=("Your username."),
+        help=user_help,
         type=str,
         required=not configured("user") and api_requires("user"),
         default=default("user"),
@@ -732,7 +805,7 @@ def _create_base_parsers():
     base_parser.add_argument(
         "-o",
         "--org-name",
-        help="Name of the target organization",
+        help=org_name_help,
         type=str,
         required=not configured("org_name") and api_requires("org_name"),
         default=default("org_name"),
@@ -740,22 +813,13 @@ def _create_base_parsers():
     base_parser.add_argument(
         "-bu",
         "--base-url",
-        help=(
-            "Base url to a platform API. Must be HTTPS. For example, with "
-            "github.com, the base url is https://api.github.com, and with "
-            "GitHub enterprise, the url is https://<ENTERPRISE_HOST>/api/v3"
-        ),
+        help=base_url_help,
         type=str,
         required=not configured("base_url") and api_requires("base_url"),
         default=default("base_url"),
     )
     base_parser.add_argument(
-        "-t",
-        "--token",
-        help="OAUTH token for the platform instance. Can also be specified in "
-        "the `REPOBEE_OAUTH` environment variable.",
-        type=str,
-        default=default("token"),
+        "-t", "--token", help=token_help, type=str, default=default("token")
     )
 
     base_parser.add_argument(
@@ -772,7 +836,7 @@ def _create_base_parsers():
     students.add_argument(
         "-sf",
         "--students-file",
-        help="Path to a list of student usernames.",
+        help=students_file_help,
         type=str,
         default=default("students_file"),
     )
@@ -788,9 +852,7 @@ def _create_base_parsers():
     master_org_parser.add_argument(
         "-mo",
         "--master-org-name",
-        help="Name of the organization containing the master repos. "
-        "Defaults to the same value as `-o|--org-name` if left unspecified. "
-        "Note that config values take precedence over this default.",
+        help=master_org_help,
         default=default("master_org_name"),
     )
 
@@ -915,8 +977,9 @@ def _repo_names_to_urls(
     return non_local_urls + local_uris
 
 
-def parse_plugins(sys_args: Tuple[str]):
-    """Parse all plugin arguments.
+def parse_preparser_options(sys_args: Tuple[str]):
+    """Parse all arguments that can somehow alter the end-user CLI, such
+    as plugins.
 
     Args:
         sys_args: Command line arguments.
@@ -925,16 +988,21 @@ def parse_plugins(sys_args: Tuple[str]):
         prog="repobee", description="plugin pre-parser for repobee."
     )
 
-    mutex_grp = parser.add_mutually_exclusive_group(required=True)
+    mutex_grp = parser.add_mutually_exclusive_group()
     mutex_grp.add_argument(
-        "-p",
-        "--plug",
+        *PRE_PARSER_PLUG_OPTS,
         help="Specify the name of a plugin to use.",
         type=str,
         action="append",
+        default=None
     )
     mutex_grp.add_argument(
-        "--no-plugins", help="Disable plugins.", action="store_true"
+        PRE_PARSER_NO_PLUGS, help="Disable plugins.", action="store_true"
+    )
+    mutex_grp.add_argument(
+        PRE_PARSER_SHOW_ALL_OPTS,
+        help="Unsuppress all options in help menus",
+        action="store_true",
     )
 
     args = parser.parse_args(sys_args)
diff --git a/repobee/main.py b/repobee/main.py
index 90e78cb..ce30b43 100644
--- a/repobee/main.py
+++ b/repobee/main.py
@@ -28,20 +28,20 @@ and supply the stack trace below.""".replace(
 
 
 def _separate_args(args: List[str]) -> (List[str], List[str]):
-    """Separate args into plugin args and repobee args."""
-    plugin_args = []
-    if args and (args[0].startswith("-p") or "plug" in args[0]):
+    """Separate args into preparser args and repobee args."""
+    preparser_args = []
+    if args and args[0].startswith("-"):
         cur = 0
         while cur < len(args) and args[cur].startswith("-"):
-            if args[cur].startswith("-p"):
-                plugin_args += args[cur : cur + 2]
+            if args[cur] in cli.PRE_PARSER_PLUG_OPTS:
+                preparser_args += args[cur : cur + 2]
                 cur += 2
-            elif args[cur] == "--no-plugins":
-                plugin_args.append(args[cur])
+            elif args[cur] in cli.PRE_PARSER_FLAGS:
+                preparser_args.append(args[cur])
                 cur += 1
             else:
                 break
-    return plugin_args, args[len(plugin_args) :]
+    return preparser_args, args[len(preparser_args) :]
 
 
 def main(sys_args: List[str]):
@@ -50,17 +50,18 @@ def main(sys_args: List[str]):
     traceback = False
     pre_init = True
     try:
-        plugin_args, app_args = _separate_args(args)
+        preparser_args, app_args = _separate_args(args)
+        parsed_preparser_args = cli.parse_preparser_options(preparser_args)
 
-        if plugin_args:
-            parsed_plugin_args = cli.parse_plugins(plugin_args)
-            if parsed_plugin_args.no_plugins:
-                LOGGER.info("Plugins disabled")
-            else:
-                plugin.initialize_plugins(parsed_plugin_args.plug)
-        else:
+        if parsed_preparser_args.no_plugins:
+            LOGGER.info("Non-default plugins disabled")
             plugin.initialize_plugins()
-        parsed_args, api = cli.parse_args(app_args)
+        else:
+            plugin.initialize_plugins(parsed_preparser_args.plug or [])
+
+        parsed_args, api = cli.parse_args(
+            app_args, show_all_opts=parsed_preparser_args.show_all_opts
+        )
         traceback = parsed_args.traceback
         pre_init = False
         cli.dispatch_command(parsed_args, api)


Test patch:
diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py
index b685b70..2ceb94c 100644
--- a/tests/unit_tests/test_cli.py
+++ b/tests/unit_tests/test_cli.py
@@ -313,6 +313,36 @@ def test_help_calls_add_arguments(monkeypatch, parser):
 class TestBaseParsing:
     """Test the basic functionality of parsing."""
 
+    def test_show_all_opts_true_shows_configured_args(
+        self, config_mock, capsys
+    ):
+        """Test that configured args are shown when show_all_opts is True."""
+        with pytest.raises(SystemExit):
+            cli.parse_args([cli.SETUP_PARSER, "-h"], show_all_opts=True)
+
+        captured = capsys.readouterr()
+        assert "--user" in captured.out
+        assert "--base-url" in captured.out
+        assert "--org-name" in captured.out
+        assert "--master-org-name" in captured.out
+        assert "--students-file" in captured.out
+        assert "--token" in captured.out
+
+    def test_show_all_opts_false_hides_configured_args(
+        self, config_mock, capsys
+    ):
+        """Test that configured args are hidden when show_all_opts is False."""
+        with pytest.raises(SystemExit):
+            cli.parse_args([cli.SETUP_PARSER, "-h"], show_all_opts=False)
+
+        captured = capsys.readouterr()
+        assert "--user" not in captured.out
+        assert "--base-url" not in captured.out
+        assert "--org-name" not in captured.out
+        assert "--master-org-name" not in captured.out
+        assert "--students-file" not in captured.out
+        assert "--token" not in captured.out
+
     def test_raises_on_invalid_org(self, api_class_mock, students_file):
         """Test that an appropriate error is raised when the organization is
         not found.
diff --git a/tests/unit_tests/test_main.py b/tests/unit_tests/test_main.py
index b7a845f..fa6cc37 100644
--- a/tests/unit_tests/test_main.py
+++ b/tests/unit_tests/test_main.py
@@ -1,3 +1,4 @@
+import argparse
 from unittest.mock import MagicMock
 from collections import namedtuple
 
@@ -45,10 +46,13 @@ def api_instance_mock(mocker):
 
 @pytest.fixture
 def init_plugins_mock(mocker):
+    def init_plugins(plugs=None):
+        list(map(module, plugs or []))
+
     return mocker.patch(
         "repobee.plugin.initialize_plugins",
         autospec=True,
-        side_effect=lambda plugs: list(map(module, plugs)),
+        side_effect=init_plugins,
     )
 
 
@@ -62,14 +66,8 @@ def parse_args_mock(mocker, api_instance_mock):
 
 
 @pytest.fixture
-def parse_plugins_mock(mocker):
-    return mocker.patch(
-        "repobee.cli.parse_plugins",
-        autospec=True,
-        side_effect=lambda args: [
-            arg for arg in args if not arg.startswith("-")
-        ],
-    )
+def parse_preparser_options_mock(mocker):
+    return mocker.patch("repobee.cli.parse_preparser_options", autospec=True)
 
 
 @pytest.fixture
@@ -85,7 +83,7 @@ def test_happy_path(
 
     main.main(sys_args)
 
-    parse_args_mock.assert_called_once_with(sys_args[1:])
+    parse_args_mock.assert_called_once_with(sys_args[1:], show_all_opts=False)
     dispatch_command_mock.assert_called_once_with(
         PARSED_ARGS, api_instance_mock
     )
@@ -101,7 +99,7 @@ def test_does_not_raise_on_exception_in_parsing(
 
     main.main(sys_args)
 
-    parse_args_mock.assert_called_once_with(sys_args[1:])
+    parse_args_mock.assert_called_once_with(sys_args[1:], show_all_opts=False)
     assert not dispatch_command_mock.called
 
 
@@ -125,18 +123,21 @@ def test_plugins_args(
     main.main(sys_args)
 
     init_plugins_mock.assert_called_once_with(["javac", "pylint"])
-    parse_args_mock.assert_called_once_with(CLONE_ARGS)
+    parse_args_mock.assert_called_once_with(CLONE_ARGS, show_all_opts=False)
 
 
 def test_no_plugins_arg(
     parse_args_mock, dispatch_command_mock, init_plugins_mock
 ):
+    """Default plugins still need to be loaded, so initialize_plugins should be
+    called without arguments.
+    """
     sys_args = ["repobee", "--no-plugins", *CLONE_ARGS]
 
     main.main(sys_args)
 
-    assert not init_plugins_mock.called
-    parse_args_mock.assert_called_once_with(CLONE_ARGS)
+    init_plugins_mock.assert_called_once_with()
+    parse_args_mock.assert_called_once_with(CLONE_ARGS, show_all_opts=False)
 
 
 def test_plugin_with_subparser_name(
@@ -147,7 +148,7 @@ def test_plugin_with_subparser_name(
     main.main(sys_args)
 
     init_plugins_mock.assert_called_once_with(["javac", "clone"])
-    parse_args_mock.assert_called_once_with(CLONE_ARGS)
+    parse_args_mock.assert_called_once_with(CLONE_ARGS, show_all_opts=False)
 
 
 def test_plug_arg_incompatible_with_no_plugins(
@@ -215,4 +216,37 @@ def test_logs_traceback_on_exception_in_dispatch_if_traceback(
     main.main(sys_args)
 
     assert logger_exception_mock.called
-    parse_args_mock.assert_called_once_with([*CLONE_ARGS, "--traceback"])
+    parse_args_mock.assert_called_once_with(
+        [*CLONE_ARGS, "--traceback"], show_all_opts=False
+    )
+
+
+def test_show_all_opts_correctly_separated(
+    parse_args_mock, parse_preparser_options_mock, no_config_mock
+):
+    msg = "expected exit"
+
+    def _raise_sysexit(*args, **kwargs):
+        raise SystemExit(msg)
+
+    parse_preparser_options_mock.return_value = argparse.Namespace(
+        show_all_opts=True, no_plugins=False, plug=None
+    )
+    parse_args_mock.side_effect = _raise_sysexit
+    sys_args = [
+        "repobee",
+        cli.PRE_PARSER_SHOW_ALL_OPTS,
+        cli.SETUP_PARSER,
+        "-h",
+    ]
+
+    with pytest.raises(SystemExit) as exc_info:
+        main.main(sys_args)
+
+    assert msg in str(exc_info.value)
+    parse_args_mock.assert_called_once_with(
+        [cli.SETUP_PARSER, "-h"], show_all_opts=True
+    )
+    parse_preparser_options_mock.assert_called_once_with(
+        [cli.PRE_PARSER_SHOW_ALL_OPTS]
+    )