Repository: repobee/repobee

Issue:
Reorder CLI arguments
Some of the commands have really weird ordering of CLI arguments. They should be reordered such that the important settings come first, to the extent that's possible.

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

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

```diff
@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
- Coverage    91.4%   91.34%   -0.07%     
==========================================
  Files          19       19              
  Lines        1315     1328      +13     
  Branches      238      241       +3     
==========================================
+ Hits         1202     1213      +11     
- Misses         93       95       +2     
  Partials       20       20
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/236?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/236/diff?src=pr&el=tree#diff-cmVwb2JlZS9jbGkucHk=) | `95.63% <100%> (-0.62%)` | :arrow_down: |
| [repobee/config.py](https://codecov.io/gh/repobee/repobee/pull/236/diff?src=pr&el=tree#diff-cmVwb2JlZS9jb25maWcucHk=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/236?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/236?src=pr&el=footer). Last update [84c67ec...363204c](https://codecov.io/gh/repobee/repobee/pull/236?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 6732cef..6b4abcc 100644
--- a/repobee/cli.py
+++ b/repobee/cli.py
@@ -83,7 +83,6 @@ PURGE_REVIEW_TEAMS_PARSER = "purge-review-teams"
 CHECK_REVIEW_PROGRESS_PARSER = "check-reviews"
 SHOW_CONFIG_PARSER = "show-config"
 
-
 PARSER_NAMES = (
     SETUP_PARSER,
     UPDATE_PARSER,
@@ -348,6 +347,7 @@ def _add_peer_review_parsers(base_parsers, subparsers):
         ),
         help="Randomly assign students to peer review each others' repos.",
         parents=base_parsers,
+        formatter_class=_OrderedFormatter,
     )
     assign_parser.add_argument(
         "-n",
@@ -376,6 +376,7 @@ def _add_peer_review_parsers(base_parsers, subparsers):
         help="Remove all review teams associated with the specified "
         "students and master repos.",
         parents=base_parsers,
+        formatter_class=_OrderedFormatter,
     )
     check_review_progress = subparsers.add_parser(
         CHECK_REVIEW_PROGRESS_PARSER,
@@ -393,6 +394,7 @@ def _add_peer_review_parsers(base_parsers, subparsers):
             "have been opened)."
         ),
         parents=base_parsers,
+        formatter_class=_OrderedFormatter,
     )
     check_review_progress.add_argument(
         "-r",
@@ -429,6 +431,7 @@ def _add_issue_parsers(base_parsers, subparsers):
         ),
         help="Open issues in student repos.",
         parents=base_parsers,
+        formatter_class=_OrderedFormatter,
     )
     open_parser.add_argument(
         "-i",
@@ -448,6 +451,7 @@ def _add_issue_parsers(base_parsers, subparsers):
         ),
         help="Close issues in student repos.",
         parents=base_parsers,
+        formatter_class=_OrderedFormatter,
     )
     close_parser.add_argument(
         "-r",
@@ -465,6 +469,7 @@ def _add_issue_parsers(base_parsers, subparsers):
         description="List issues in student repos.",
         help="List issues in student repos.",
         parents=base_parsers,
+        formatter_class=_OrderedFormatter,
     )
     list_parser.add_argument(
         "-r",
@@ -512,6 +517,51 @@ def _add_issue_parsers(base_parsers, subparsers):
     list_parser.set_defaults(state="open")
 
 
+class _OrderedFormatter(argparse.HelpFormatter):
+    """A formatter class for putting out the help section in a proper order.
+    All of the arguments that are configurable in the configuration file
+    should appear at the bottom (in arbitrary, but always the same, order).
+    Any other arguments should appear in the order they are added.
+
+    The internals of the formatter classes are technically not public,
+    so this class is "unsafe" when it comes to new versions of Python. It may
+    have to be disabled for future versions, but it works for 3.5, 3.6 and 3.7
+    at the time of writing. If this turns troublesome, it may be time to
+    switch to some other CLI library.
+    """
+
+    def add_arguments(self, actions):
+        """Order actions by the name  of the long argument, and then add them
+        as arguments.
+
+        The order is the following:
+
+        [ NON-CONFIGURABLE | CONFIGURABLE | DEBUG ]
+
+        Non-configurable arguments added without modification, which by
+        default is the order they are added to the parser. Configurable
+        arguments are added in the order defined by
+        :py:const:`config.ORDERED_CONFIGURABLE_ARGS`. Finally, debug commands
+        (such as ``--traceback``) are added in arbitrary (but consistent)
+        order.
+        """
+        args_order = tuple(
+            "--" + name.replace("_", "-")
+            for name in config.ORDERED_CONFIGURABLE_ARGS
+        ) + ("--traceback",)
+
+        def key(action):
+            if len(action.option_strings) < 2:
+                return -1
+            long_arg = action.option_strings[1]
+            if long_arg in args_order:
+                return args_order.index(long_arg)
+            return -1
+
+        actions = sorted(actions, key=key)
+        super().add_arguments(actions)
+
+
 def _create_parser():
     """Create the parser."""
 
@@ -522,6 +572,7 @@ def _create_parser():
             "on GitHub instances. See the full documentation at "
             "https://repobee.readthedocs.io"
         ),
+        formatter_class=_OrderedFormatter,
     )
     parser.add_argument(
         "-v",
@@ -570,6 +621,7 @@ def _add_subparsers(parser):
             "file can be found, show the path where repobee expectes to find "
             "it."
         ),
+        formatter_class=_OrderedFormatter,
     )
 
     subparsers.add_parser(
@@ -590,6 +642,7 @@ def _add_subparsers(parser):
             master_org_parser,
             repo_name_parser,
         ],
+        formatter_class=_OrderedFormatter,
     )
 
     update = subparsers.add_parser(
@@ -602,6 +655,7 @@ def _add_subparsers(parser):
             master_org_parser,
             repo_name_parser,
         ],
+        formatter_class=_OrderedFormatter,
     )
     update.add_argument(
         "-i",
@@ -622,6 +676,7 @@ def _add_subparsers(parser):
             "migrated repos will be private."
         ),
         parents=[repo_name_parser, base_user_parser],
+        formatter_class=_OrderedFormatter,
     )
 
     clone = subparsers.add_parser(
@@ -629,6 +684,7 @@ def _add_subparsers(parser):
         help="Clone student repos.",
         description="Clone student repos asynchronously in bulk.",
         parents=[base_student_parser, repo_name_parser],
+        formatter_class=_OrderedFormatter,
     )
 
     plug.manager.hook.clone_parser_hook(clone_parser=clone)
@@ -655,6 +711,7 @@ def _add_subparsers(parser):
             "message is displayed."
         ),
         parents=[base_parser, base_user_parser, master_org_parser],
+        formatter_class=_OrderedFormatter,
     )
 
 
diff --git a/repobee/config.py b/repobee/config.py
index 831d3c6..eb6526b 100644
--- a/repobee/config.py
+++ b/repobee/config.py
@@ -30,17 +30,16 @@ DEFAULT_CONFIG_FILE = CONFIG_DIR / "config.cnf"
 assert DEFAULT_CONFIG_FILE.is_absolute()
 
 # arguments that can be configured via config file
-CONFIGURABLE_ARGS = set(
-    (
-        "user",
-        "org_name",
-        "github_base_url",
-        "students_file",
-        "plugins",
-        "master_org_name",
-        "token",
-    )
+ORDERED_CONFIGURABLE_ARGS = (
+    "user",
+    "org_name",
+    "github_base_url",
+    "students_file",
+    "plugins",
+    "master_org_name",
+    "token",
 )
+CONFIGURABLE_ARGS = set(ORDERED_CONFIGURABLE_ARGS)
 
 
 def get_configured_defaults(


Test patch:
diff --git a/tests/test_cli.py b/tests/test_cli.py
index 82cf186..8bb9f8a 100644
--- a/tests/test_cli.py
+++ b/tests/test_cli.py
@@ -289,6 +289,30 @@ class TestDispatchCommand:
         )
 
 
+@pytest.mark.parametrize("parser", cli.PARSER_NAMES)
+def test_help_calls_add_arguments(monkeypatch, parser):
+    """Test that the --help command causes _OrderedFormatter.add_arguments to
+    be called. The reason this may not be the case is that
+    HelpFormatter.add_arguments is not technically public, and so it could be
+    removed or changed in future versions of Python.
+    """
+    called = False
+    add_arguments = cli._OrderedFormatter.add_arguments
+
+    def wrapper(self, *args, **kwargs):
+        nonlocal called
+        called = True
+        add_arguments(self, *args, **kwargs)
+
+    monkeypatch.setattr("repobee.cli._OrderedFormatter.add_arguments", wrapper)
+
+    with pytest.raises(SystemExit) as exc_info:
+        cli.parse_args([parser, "--help"])
+
+    assert exc_info.value.code == 0
+    assert called
+
+
 class TestBaseParsing:
     """Test the basic functionality of parsing."""