Fetch and pull branches if local copies of student repos already exist #89

LinuxMercedes · 2018-01-15T23:26:00Z

Also, switch to tqdm for progress reporting...not sure I'm happy with it though.

Closes #11.

LinuxMercedes · 2018-01-16T08:11:05Z

@brhoades I re-did the progress bar method; this is a bit less bad I think?

brhoades · 2018-01-16T19:14:53Z

assigner/commands/get.py

+
+    progress = tqdm()
+
+    for i,student in progress.enumerate(roster):


np: space after commas

brhoades · 2018-01-16T19:20:07Z

assigner/commands/get.py


        try:
            repo = StudentRepo(host, namespace, full_name, token)
-            repo.clone_to(os.path.join(path, username))
-            count += 1
+            repo_dir = os.path.join(path, username)


I think you can split here into another method without too many args... let me look:
somefunction(i + 1, repo, **student)

Am I missing something? That'll clean up the multiline variable definitions too maybe?

I'm not sure it's worth it. Moving this would only make it more complicated.

Yeah I'm loathe to split a function just for the sake of splitting a function.

brhoades · 2018-01-16T19:23:43Z

assigner/commands/get.py

+            name = student["name"]
+
+            try:
+                logging.debug("Attempting to use local repo {}...".format(repo_dir))


I'm starting to wonder if we should switch to logging's placeholders:

logging.debug("Attempting to use local repo \"%(local_repo)\"", local_repo=repo_dir)

No, I don't expect you to change it. Should we open an issue or just go with format 🤔

This has the advantage of unpacking kwargs and providing debug data more easily.

This does look pretty nice...let me do that at least here.

brhoades · 2018-01-16T19:26:31Z

assigner/commands/get.py

+                for result in results:
+                    logging.debug("fetch result: {} {} {}".format(result.ref.name, result.flags, result.note))
+
+                    if result.flags & result.NEW_HEAD:


🤢
I'm surprised and upset that this is the way to do this?

That's what you get for using a library that doesn't wrap git tightly enough!

I will document this though.

brhoades · 2018-01-16T19:27:02Z

assigner/commands/get.py

+
+                    if result.flags & result.NEW_HEAD:
+                        output.add_row([row, sec, sid, name, "{}: new branch at {}".format(
+                            result.ref.name, str(result.ref.commit)[0:8]


np 9000: str(result.ref.commit)[:8]

brhoades · 2018-01-16T19:29:20Z

assigner/tqdm.py

+        yield orig_out_err[0]
+    # Relay exceptions
+    except Exception as exc:
+        raise exc


Woah, this isn't the default?

TOO AWESOME 💥

brhoades · 2018-01-16T19:29:48Z

assigner/tqdm.py

+
+def tqdm_enumerate(iterable, stdout):
+    return enumerate(_tqdm(iterable, file=stdout, dynamic_ncols=True))
+


two spaces between top level definitions.

brhoades · 2018-01-16T19:30:19Z

assigner/tqdm.py

+            _tqdm.write(x, file=self.file)
+
+    def flush(self):
+        return getattr(self.file, "flush", lambda: None)()


😵 😵 😵 😵 😵 😵

Send documentation 🚑

brhoades · 2018-01-16T19:33:01Z

assigner/commands/get.py

+                        )])
+                        row = sec = sid = name = ""
+
+


np: too many spaces. I'd accept a space and a comment :)

brhoades · 2018-01-16T19:33:58Z

assigner/tqdm.py

+    """Dummy file-like that will write to tqdm"""
+    file = None
+    def __init__(self, file):
+        self.file = file


I recognize that there are three files here but they're all named the same thing. Can we rename them?

brhoades · 2018-01-16T19:38:18Z

assigner/baserepo.py

@@ -170,6 +170,25 @@ def already_exists(self):
            return True
        return False

+    def get_head(self, branch):
+        if self.repo is None:
+            raise RepoError("No repo to get head from")


Do we do punctuation on errors?

Not that I can see, although we haven't really made a standard for that.

brhoades · 2018-01-16T19:48:04Z

assigner/commands/get.py

+                        logging.warn("  (use --force to overwrite)")
+
+                # Check out first branch specified; this is probably what people expect
+                if len(branch) > 1:


What about branches == 1? Am I missing something.

If there's just one branch, then it'll be the last thing checked out by the for loop above.

brhoades · 2018-01-16T19:49:14Z

assigner/commands/get.py

        username = student["username"]
        student_section = student["section"]
        full_name = StudentRepo.name(semester, student_section,
-                                     hw_name, username)
+                                    hw_name, username)


continuation should follow the opening brace, + 1 space.

brhoades · 2018-01-17T15:01:14Z

assigner/commands/get.py

+                logging.debug("Local repo exists, fetching...")
+                results = repo.repo.remote().fetch()
+                for result in results:
+                    logging.debug("fetch result: name: %s flags: %s note: %s", result.ref.name, result.flags, result.note)


too long or is char limit set at 120?

LinuxMercedes requested a review from brhoades January 15, 2018 23:26

LinuxMercedes force-pushed the do-git-pull branch from 323d745 to c2daaeb Compare January 16, 2018 08:09

LinuxMercedes force-pushed the do-git-pull branch from c2daaeb to 297b16b Compare January 16, 2018 19:22

brhoades requested changes Jan 16, 2018

View reviewed changes

LinuxMercedes force-pushed the do-git-pull branch from 297b16b to 600e388 Compare January 16, 2018 19:36

brhoades reviewed Jan 16, 2018

View reviewed changes

LinuxMercedes force-pushed the do-git-pull branch 2 times, most recently from f3268be to 9d93bfd Compare January 16, 2018 20:03

brhoades reviewed Jan 17, 2018

View reviewed changes

brhoades approved these changes Jan 17, 2018

View reviewed changes

LinuxMercedes added 4 commits January 17, 2018 09:20

Fetch and pull branches if local copies of student repos already exist

8880bbc

Switch to tqdm for progress bar

f132d03

Changelog update

e633668

Appease pylint

fb158e4

LinuxMercedes force-pushed the do-git-pull branch from 399e0aa to fb158e4 Compare January 17, 2018 15:20

LinuxMercedes merged commit 427f558 into master Jan 17, 2018

LinuxMercedes deleted the do-git-pull branch February 7, 2018 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch and pull branches if local copies of student repos already exist #89

Fetch and pull branches if local copies of student repos already exist #89

LinuxMercedes commented Jan 15, 2018

LinuxMercedes commented Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

LinuxMercedes Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

LinuxMercedes Jan 16, 2018

brhoades Jan 16, 2018

LinuxMercedes Jan 16, 2018

LinuxMercedes Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 16, 2018

LinuxMercedes Jan 16, 2018

brhoades Jan 16, 2018

LinuxMercedes Jan 16, 2018

brhoades Jan 16, 2018

brhoades Jan 17, 2018


		progress = tqdm()

		for i,student in progress.enumerate(roster):


		def tqdm_enumerate(iterable, stdout):
		return enumerate(_tqdm(iterable, file=stdout, dynamic_ncols=True))

Fetch and pull branches if local copies of student repos already exist #89

Fetch and pull branches if local copies of student repos already exist #89

Conversation

LinuxMercedes commented Jan 15, 2018

LinuxMercedes commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment