Repository: repobee/repobee

Issue:
Add `user` to base args
See #180 

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

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

```diff
@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
- Coverage   89.92%   89.72%   -0.21%     
==========================================
  Files          19       19              
  Lines        1410     1411       +1     
  Branches      255      256       +1     
==========================================
- Hits         1268     1266       -2     
- Misses        119      123       +4     
+ Partials       23       22       -1
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/281?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/281/diff?src=pr&el=tree#diff-cmVwb2JlZS9jbGkucHk=) | `97.65% <100%> (-0.79%)` | :arrow_down: |
| [repobee/ext/github.py](https://codecov.io/gh/repobee/repobee/pull/281/diff?src=pr&el=tree#diff-cmVwb2JlZS9leHQvZ2l0aHViLnB5) | `92.09% <33.33%> (-0.74%)` | :arrow_down: |
| [repobee/ext/gitlab.py](https://codecov.io/gh/repobee/repobee/pull/281/diff?src=pr&el=tree#diff-cmVwb2JlZS9leHQvZ2l0bGFiLnB5) | `64.59% <0%> (ø)` | :arrow_up: |
| [repobee/command.py](https://codecov.io/gh/repobee/repobee/pull/281/diff?src=pr&el=tree#diff-cmVwb2JlZS9jb21tYW5kLnB5) | `95.26% <0%> (+0.59%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/281?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/281?src=pr&el=footer). Last update [ee949e1...dc59df9](https://codecov.io/gh/repobee/repobee/pull/281?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 3b33755..7ff3eae 100644
--- a/repobee/cli.py
+++ b/repobee/cli.py
@@ -585,13 +585,11 @@ def _add_subparsers(parser):
     `base_` prefixed parser, of course).
     """
 
-    base_parser, base_student_parser, base_user_parser, master_org_parser = (
+    base_parser, base_student_parser, master_org_parser = (
         _create_base_parsers()
     )
 
-    repo_name_parser = argparse.ArgumentParser(
-        add_help=False, parents=[base_parser]
-    )
+    repo_name_parser = argparse.ArgumentParser(add_help=False)
     repo_name_parser.add_argument(
         "-mn",
         "--master-repo-names",
@@ -619,7 +617,7 @@ def _add_subparsers(parser):
             "will simply be skipped."
         ),
         parents=[
-            base_user_parser,
+            base_parser,
             base_student_parser,
             master_org_parser,
             repo_name_parser,
@@ -637,7 +635,7 @@ def _add_subparsers(parser):
             "something already)."
         ),
         parents=[
-            base_user_parser,
+            base_parser,
             base_student_parser,
             master_org_parser,
             repo_name_parser,
@@ -662,7 +660,7 @@ def _add_subparsers(parser):
             "The repos must be local on disk to be migrated. Note that "
             "migrated repos will be private."
         ),
-        parents=[repo_name_parser, base_user_parser],
+        parents=[repo_name_parser, base_parser],
         formatter_class=_OrderedFormatter,
     )
 
@@ -670,15 +668,17 @@ def _add_subparsers(parser):
         CLONE_PARSER,
         help="Clone student repos.",
         description="Clone student repos asynchronously in bulk.",
-        parents=[base_student_parser, repo_name_parser],
+        parents=[base_parser, base_student_parser, repo_name_parser],
         formatter_class=_OrderedFormatter,
     )
 
     plug.manager.hook.clone_parser_hook(clone_parser=clone)
 
-    _add_issue_parsers([base_student_parser, repo_name_parser], subparsers)
+    _add_issue_parsers(
+        [base_parser, base_student_parser, repo_name_parser], subparsers
+    )
     _add_peer_review_parsers(
-        [base_student_parser, repo_name_parser], subparsers
+        [base_parser, base_student_parser, repo_name_parser], subparsers
     )
 
     subparsers.add_parser(
@@ -696,7 +696,7 @@ def _add_subparsers(parser):
         VERIFY_PARSER,
         help="Verify core settings.",
         description="Verify core settings by trying various API requests.",
-        parents=[base_parser, base_user_parser, master_org_parser],
+        parents=[base_parser, master_org_parser],
         formatter_class=_OrderedFormatter,
     )
 
@@ -720,6 +720,15 @@ def _create_base_parsers():
         return arg_name in plug.manager.hook.api_init_requires()
 
     base_parser = argparse.ArgumentParser(add_help=False)
+    base_parser.add_argument(
+        "-u",
+        "--user",
+        help=("Your username."),
+        type=str,
+        required=not configured("user") and api_requires("user"),
+        default=default("user"),
+    )
+
     base_parser.add_argument(
         "-o",
         "--org-name",
@@ -775,19 +784,6 @@ def _create_base_parsers():
         nargs="+",
     )
 
-    # base parser for when files need to be pushed
-    base_user_parser = argparse.ArgumentParser(add_help=False)
-
-    # the username is required for any pushing
-    base_user_parser.add_argument(
-        "-u",
-        "--user",
-        help=("Your username."),
-        type=str,
-        required=not configured("user") and api_requires("user"),
-        default=default("user"),
-    )
-
     master_org_parser = argparse.ArgumentParser(add_help=False)
     master_org_parser.add_argument(
         "-mo",
@@ -798,12 +794,7 @@ def _create_base_parsers():
         default=default("master_org_name"),
     )
 
-    return (
-        base_parser,
-        base_student_parser,
-        base_user_parser,
-        master_org_parser,
-    )
+    return (base_parser, base_student_parser, master_org_parser)
 
 
 @contextmanager
diff --git a/repobee/ext/github.py b/repobee/ext/github.py
index 2a58236..09b6824 100644
--- a/repobee/ext/github.py
+++ b/repobee/ext/github.py
@@ -129,6 +129,8 @@ class GitHubAPI(apimeta.API):
             user: Name of the current user of the API.
             org_name: Name of the target organization.
         """
+        if not user:
+            raise TypeError("argument 'user' must not be empty")
         self._github = github.Github(login_or_token=token, base_url=base_url)
         self._org_name = org_name
         self._base_url = base_url
@@ -381,13 +383,7 @@ class GitHubAPI(apimeta.API):
                     repo_url
                 )
             )
-        # TODO in RepoBee 2.0, user will always be available
-        # user may not always be available
-        auth = (
-            self.token
-            if not self._user
-            else "{}:{}".format(self._user, self.token)
-        )
+        auth = "{}:{}".format(self._user, self.token)
         return repo_url.replace("https://", "https://{}@".format(auth))
 
     def get_issues(


Test patch:
diff --git a/tests/unit_tests/plugin_tests/test_github.py b/tests/unit_tests/plugin_tests/test_github.py
index a67017f..e55e41b 100644
--- a/tests/unit_tests/plugin_tests/test_github.py
+++ b/tests/unit_tests/plugin_tests/test_github.py
@@ -479,24 +479,6 @@ class TestCreateRepos:
 class TestGetRepoUrls:
     """Tests for get_repo_urls."""
 
-    @pytest.mark.skipif(
-        repobee.__version__ >= "2.0.0",
-        reason="Made obsolete by including user in every CLI command",
-    )
-    def test_with_token_without_user(self, repos, api):
-        """Test getting repo urls when the api has the token, but no user. The
-        token should be in the url anyway.
-        """
-        api._user = None
-        repo_names = [repo.name for repo in repos]
-        expected_urls = [api._insert_auth(repo.html_url) for repo in repos]
-
-        urls = api.get_repo_urls(repo_names)
-
-        assert sorted(urls) == sorted(expected_urls)
-        for url in urls:
-            assert TOKEN in url
-
     def test_with_token_and_user(self, repos, api):
         repo_names = [repo.name for repo in repos]
         api._user = USER
diff --git a/tests/unit_tests/test_cli.py b/tests/unit_tests/test_cli.py
index d8b16a2..b685b70 100644
--- a/tests/unit_tests/test_cli.py
+++ b/tests/unit_tests/test_cli.py
@@ -31,8 +31,8 @@ TOKEN = constants.TOKEN
 REPO_NAMES = ("week-1", "week-2", "week-3")
 REPO_URLS = tuple(map(lambda rn: generate_repo_url(rn, ORG_NAME), REPO_NAMES))
 
-BASE_ARGS = ["-bu", BASE_URL, "-o", ORG_NAME]
-BASE_PUSH_ARGS = ["-u", USER, "-mn", *REPO_NAMES]
+BASE_ARGS = ["-u", USER, "-bu", BASE_URL, "-o", ORG_NAME]
+BASE_PUSH_ARGS = ["-mn", *REPO_NAMES]
 COMPLETE_PUSH_ARGS = [*BASE_ARGS, *BASE_PUSH_ARGS]
 
 # parsed args without subparser
@@ -459,6 +459,8 @@ class TestBaseParsing:
         """
         sys_args = [
             cli.SETUP_PARSER,
+            "-u",
+            USER,
             "-o",
             ORG_NAME,
             "-bu",
@@ -809,14 +811,7 @@ class TestMigrateParser:
         assert parsed_args.master_repo_urls == self.LOCAL_URIS
 
     def test_happy_path(self):
-        sys_args = [
-            cli.MIGRATE_PARSER,
-            *BASE_ARGS,
-            "-u",
-            USER,
-            "-mn",
-            *self.NAMES,
-        ]
+        sys_args = [cli.MIGRATE_PARSER, *BASE_ARGS, "-mn", *self.NAMES]
 
         parsed_args, _ = cli.parse_args(sys_args)
 
@@ -827,7 +822,7 @@ class TestVerifyParser:
     """Tests for the VERIFY_PARSER."""
 
     def test_happy_path(self):
-        sys_args = [cli.VERIFY_PARSER, *BASE_ARGS, "-u", USER]
+        sys_args = [cli.VERIFY_PARSER, *BASE_ARGS]
 
         args, _ = cli.parse_args(sys_args)
 
@@ -871,25 +866,11 @@ class TestCloneParser:
         [
             (
                 cli.SETUP_PARSER,
-                [
-                    "-u",
-                    USER,
-                    "-s",
-                    *STUDENTS_STRING.split(),
-                    "-mn",
-                    *REPO_NAMES,
-                ],
+                ["-s", *STUDENTS_STRING.split(), "-mn", *REPO_NAMES],
             ),
             (
                 cli.UPDATE_PARSER,
-                [
-                    "-u",
-                    USER,
-                    "-s",
-                    *STUDENTS_STRING.split(),
-                    "-mn",
-                    *REPO_NAMES,
-                ],
+                ["-s", *STUDENTS_STRING.split(), "-mn", *REPO_NAMES],
             ),
             (
                 cli.OPEN_ISSUE_PARSER,
@@ -913,8 +894,8 @@ class TestCloneParser:
                     "some-regex",
                 ],
             ),
-            (cli.VERIFY_PARSER, ["-u", USER]),
-            (cli.MIGRATE_PARSER, ["-u", USER, "-mn", *REPO_NAMES]),
+            (cli.VERIFY_PARSER, []),
+            (cli.MIGRATE_PARSER, ["-mn", *REPO_NAMES]),
         ],
     )
     def test_no_other_parser_gets_parse_hook(