[Refactoring] Use Typer instead of argparse?

[paketb0te]

Hi @initialcommit-io , it's me again 😆

With the inheritance model simplified, I think there is one other thing we could greatly improve - the way the command line arguments are handled.

I am a big fan of Typer, that tool makes it super easy to manage all that stuff - and it gives type hints!

The docs are also great and explain the whole concept better than I could ever do here, so maybe you want to have a look at that :)

I have worked on a branch showcasing the concept for some of the commands (log and status for now), check it out: typer

The basic idea is that each command is a function, and the function arguments are the corresponding CLI arguments / flags / options. But we don't have to do all that argparse stuff (setting up parsers and subparsers in main(), and having to sync that with the args.subcommand in the actual Command classes ...).

Let me know if you like it!

[initialcommit-io]

Hi again @paketb0te!

Wow cool! At a quick glance, I do like it. I like how it keeps just the global options in the __main__.py and the subcommand options in the individual subcommand modules. Seems easier for the workflow to have all the stuff for each subcommand in one place.

I also like that we get rid of that big if statement.

I need to take a deeper look into some of the functionality to fully understand how it works. I'll keep you posted on that but I like the direction this is going in.

Here are a few thoughts/questions:

• So for the Settings, how should we decide what should be included there? We use default values both in the global options, subcommand options that are unique to each subcommand and also in some subcommand specific __init__() methods. Would it just be anything in those categories that has a default value?

• One thing I want to make sure we can support is sub-sub commands. By that, I mean stuff like $ git stash pop and $ git stash push <filename>. I hadn't looked into that with argparse yet but it was on my agenda. It would be great to know there is a clean way to do this with Typer before switching.

• This is a small thing, but in animations.py we'd need to be able to get the command name to include in the output image/video file here: image_file_name = "git-sim-log_" + t + ".jpg"

• Also I think I like changing the git_sim_subcommand.py filenames to just subcommand.py, like log.py and status.py (as well as the corresponding class names), just seems cleaner that way. But we can keep git_sim_base_command.py as is.

[paketb0te]

So for the Settings, how should we decide what should be included there?

I'd have it store everything that is needed in the GitSimBaseCommand in there (basically replacing args - with the benefit of having type hints).

One thing I want to make sure we can support is sub-sub commands.

That is possible (and actually super easy), I have tried it already (see the subcommands section in the typer docs). You basically just create a new typer app for the subcommand and add it to the main app.

in animations.py we'd need to be able to get the command name

I think that should be fairly straightforward. Even though there does not seem to be an easy way for a function to know it's own name, we could always just add this to the Settings class or pass it explicitly to handle_animations()

I like changing the git_sim_subcommand.py filenames

Main reason was that I wanted to look at the original files while creating the examples, but if you like it yeah we can of course rename them. Maybe we even move them to a commands directory or something? 🤔

re: renaming, I think it would make sense to rename the GitSimCommand classes to GitSimCommandScene or CommandScene (because they are the Scene objects for Manim :D )

What do you think?

[paketb0te]

@initialcommit-io unfortunately this change would have to be done for all commands at once (at least I am not aware of a way to have arparse and Typer work at the same time to have a gradual change).

I am more than willing to do the work, but would like to make sure we are on the same page about this so that my time is not wasted :)

[initialcommit-io]

@paketb0te Hey sorry for the delay!

I'd have it store everything that is needed in the GitSimBaseCommand in there (basically replacing args - with the benefit of having type hints).

Ok cool - one thing to note is that some of the subcommand subclasses override properties from the GitSimBaseCommand. For example, the GitSimLog overrides self.numCommits and self.defaultNumCommits. Another example is GitSimRevert which overrides several other properties too.

Also, some subclasses might have their own new properties specific to that subcommand, that aren't implemented in the parent.

Maybe we even move them to a commands directory or something?

For now, lets keep them in the current directory until it gets too unruly 😄 . Since the command subclass modules are most of the files anyway, moving them to a subfolder wouldn't be that much cleaner for now, and would involve browsing into another subdir during development which is a bit annoying.

renaming, I think it would make sense to rename...

This makes sense, but I wonder if we should favor the cleaner approach of just naming the subcommand classes as Log, Status, Rebase, etc. I think in the context of the program it should be pretty easy to figure out they are scenes, and their main purpose is to identify the subcommand that is being represented.

unfortunately this change would have to be done for all commands at once

No worries! That's fine - and yes I do think we should move in this direction and implement this. We'll just need to make sure to test it thoroughly before releasing it. Appreciate all your time, input, and help with this I think it is a much cleaner and more intuitive way to organize this stuff.

[paketb0te]

@initialcommit-io regarding the subcommands' properties like numCommits etc, I am a bit on the fence if we should store them in a dataclass like Settings (and then reference them like Settings.commits everywhere, or if it is better to explicitly pass them as arguments to GitSimBaseCommand. I tend towards the first, do you have any preference?

(Same goes for command-specific properties)

For now, lets keep them in the current directory

Yeah makes sense 😄

naming the subcommand classes as Log, Status, Rebase, etc

Agreed, it's probably obvious enough what those are doing.

Will start working on it, but will definitely take a bit of time :)

[paketb0te]

@initialcommit-io quick question: what does the self.maxrefs in GitSimStatus do?

class GitSimStatus(GitSimBaseCommand):
    def __init__(self, args: Namespace):
        super().__init__(args=args)
        self.maxrefs = 2
        self.hide_first_tag = True
        self.allow_no_commits = True

When searching the project, I see that variable set for a few of the subcommands, but never actually accessed... is that something internal to Manim? Do we need it?

[initialcommit-io]

@paketb0te I think I used that previously to set the max number of branch/tag labels that would show on top of a single commit, but I think at some point I switched to using args.max_branches_per_commit and args.max_tags_per_commit and forgot to remove the maxrefs. So yes I think we can remove that. Good catch!

[paketb0te]

Made some progress today (long train ride), only revert, rebase, merge, cherrypick are left to change, as well as some cleaning up.

@initialcommit-io FYI, I will be away for a few weeks starting february 17th, so I try to finish everything this week, hopefully you can have a look at it over the weekend so we can get this done before I leave 🤞

[initialcommit-io]

@paketb0te Alright! I will do my best to take a look this weekend and merge the PR. Thanks again!

[initialcommit-io]

@paketb0te Just fixed a minor import bug, but aside from that seems to be working well! I haven't thoroughly tested but the basic stuff seems to be working 😄 .

Any other suggestions for this one for now before we close it out?

[initialcommit-io]

@paketb0te Hmm question about the cherrypick command/class. In Git, the command is actually cherry-pick, (with the hyphen) which is what it was using argparse. After changing to Typer, it is now set as "cherrypick" with no hyphen. Can you take a look at that?

Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/". Also just want to make sure the environment variable config that we implemented still works...

[initialcommit-io]

@paketb0te Also noticed that the shorthand options like git-sim commit -m and git-sim rebase -e and git-sim -h don't work anymore. We need to add those back in.

[paketb0te]

question about the cherrypick command/class

Ah, I was not aware of that - we can just rename the function to cherry_pick(), underscores in function names are automatically mapped to hyphens in the commands/arguments/options names :)

Also it looks like the program is creating a new folder called "media/" to put the output stuff in instead of the previous "git-sim_media/".

I just moved the relevant code into animytions.py, I did not expect that behavior
to change 🤔

On that note, I think we can improve the readability of that part by using pathlib, which can abstract the underlying OS path so we don't need to worry about forward-slash / back-slash handling manually. I'll have a look at it.

Also just want to make sure the environment variable config that we implemented still works...

There is this awesome tool pydantic which can do that for us :D

(we can just make the Settings inherit from pydantic's BaseSettings class, then we get the whole "reading env vars into variables" for free - see Settings management)