Clean up star imports

[paketb0te]

Hi @initialcommit-io,

This is a really cool project and I#d like to contribute where I can!

One thing that struck me while looking at the code was that there seem to be quite a lot of "star imports" (from foo import *), which are considered bad practice AFAIK.

They also make it much more difficult to infer what the exact types of the different objects are (since you can't have any type hints).

Would you mind if I clean those up a little bit and replace them with explicit imports?

[paketb0te]

@initialcommit-io could you help me understand what the scene parameter passed to GitSimBaseCommand's __init__() function is?

I think it is a GitSim object (since all instances are passed self on creation (in the construct() function), and the GitSim class has matching attributes / functions, but then again I don't understand what would be the point of creating an extra argument when it is always just passed self.

I am a bit confused 😅

[paketb0te]

WIP -> branch clean_star_imports on my fork

[initialcommit-io]

Thanks for this! I like what you did with git_sim.py, it is nice and clean in there. However, for the git_sim_base_command.py I don't like that we have to prefix every Manim thing with manim. I also don't really like adding an individual import for every single Manim thing (what is this, Java?! :p), so I think maybe the star import is ok to keep there... And in other files that will utilize an arbitrary number of Manim objects. Thoughts? Are there any other options you can think of for a scenario like this?

And as for the scene parameter... First in addition to the GitSimBaseCommand's __init__(), it also gets passed to all the other command class __init__() methods (such as GitSimReset, GitSimAdd, etc, since they override the init() from the base class).

And yes you're right it expects to be passed in a GitSim object, which inherits from MovingCameraScene from Manim. So yes, GitSim is actually a scene object that gets created in __main__.py on line 88:

scene = gs.GitSim(args)
scene.render()

As you can see the command line arguments args get passed in, and then GitSim makes that a local attribute to be passed into the individual command object that is being run. This gives each object the ability to access data from the scene itself from self.scene which gets set as self.scene = scene in the base command and individual command inits.

The main reason for this is so that Manim scene actions can be taken within each of the base command class and individual command classes, such as self.scene.play(...), or more generally self.scene.anything....

Note that since the arguments were added directly to the scene too (maybe better to separate those honestly...), I currently access those like self.scene.args.commits, for example.

Let me know if that makes sense. In a nutshell the reason is so that the individual commands can get access to Manim scene methods, attributes, and functionality.

[paketb0te]

I understand that we do not want to explicitly import every used object from the manim base package (I started with that, but quickly realized that it would be a looong import list...).

As for "not prefixing every Manim thing wirh manim": Would import manim as m be an acceptable middle-ground?

Easy/quick to type, but we get all the advantages of not polluting our modules namespace with a star import 😁

the explicit import makes it much easier to see where an object is defined, and it helps against accidentally overriding an object (although I think the main advantage in our case would be improved readability). Also helps with static code analysis / type hints.

Let me know what you think :)

[paketb0te]

@initialcommit-io thanks for the explanation re: the scene objects.

I think we can improve the class relationships there, I just ran into a circular import when trying to add type hints...

Could the GitSimBaseCommand inherit from Manim's MovingCameraScene directly?

Right now the GitSim class itself does nothing, it is just an intermediary to instantiate the appropriate class and store the args - both of which could be moved out.

[initialcommit-io]

Great points and suggestions. Ok - you sold me on the star imports. Let's use import manim as m as you suggested. I agree with your points about easy to type, and not polluting the namespace. Let's incorporate that in each of the files that imports Manim. It would be great if you can submit a pull request this and I will review/accept.

For the GitSim class, the main reason I set it up this way is that I was thinking of each Git command class (like GitSimLog or GitSimReset) as representing a Git command itself wit corresponding Git functionality, not representing a Manim scene.

Inheriting MovingCameraScene in each Git command class (or even just in GitSimBaseCommand) does seem tempting for the reasons you mentioned, but would imply that each Git command class is also its own scene. Maybe that's OK since this tool is created to create scenes for Git commands, as opposed to Git commands themselves?

But I kind of like the design of having 1 overarching scene object, which the Git command classes can simply modify/add-to as needed. That's why I passed the scene instance in as an argument to the init...

Thoughts? Can you think of any other way to preserve separate scene and Git command classes that would avoid the circular references you mentioned?

[paketb0te]

Hi @initialcommit-io , I opened #17 for this issue.

Regarding the class inheritance, I'll try to think of something, but I guess we can continue that discussion in a separate thread as it is not directly related to the star-imports 😃

[paketb0te]

@initialcommit-io I realize I am having difficulties wrapping my head around the nested classes and their (partially circular) references 🤪

Recently I stumbled upon Typer, which probably can simplify the whole command / subcommand / arguments stuff, but I experimented a bit with it and it would definitely require heavy changes (although I personally find the result much easier to reason about).

[initialcommit-io]

@paketb0te Hmm can you identify more specifically what you mean by "partially circular references"? The only one I can think of is what we talked about before where the GitSim class passes itself into each Git subcommand instance, like self.command = GitSimLog(self). The reason I did this was to maintain separation between GitSimCommand classes and the Scene itself. It actually makes for quite a clean setup imo.

Here is how I think about it:

Program entry point: main.py

1. collects subcommand to run and arguments based on command line input
2. initializes GitSim scene object and passes in args

git_sim.py (contains the GitSim.py scene subclass)

1. The init sets up the args to make them accessible as a class attribute
2. Manim calls .contruct() to instantiate the appropriate Git subcommand and then .execute() it
3. The GitSim object passes itself into the Git subcommand so that Manim scene methods can be performed.

git_sim_base_command:

Contains common Git-Sim command attributes and methods to be shared by subclass commands, so basically all current and future Git subcommand classes (like GitSimStatus) will inherit from GitSimBaseCommand. These inherited methods can be (and are) overridden where necessary.

git_sim_xyz:

One of these is defined for each subcommand supported by Git-Sim. In general these will override the init and execute() methods of the BaseCommand class, which specify which command-specific actions to take.

Finishing up:

Once the code in the corresponding execute() method has finished running, flow goes back to the main.py for some final logic based on whether an image or video should be generated.

---

One of the main reasons I did it this way was that originally I had all the code for each Git subcommand in it's own module, but that got to be very unruly when the number of commands grows, because every module ends up with minor changes and differences in their code, and sometimes a single change needs to be propagated to ALL files which is a nightmare for maintenance.

The current setup allows most global changes to be done to the GitSimBaseCommand class, with the exception of changes to specific methods which have been overridden by a GitSimCommand class.

So the main way I reason about the code when I need to figure out where something happens, is I open the corresponding file for the command I am working on, like GitSimAdd for "git add", then look straight at the execute() method. That will tell you exactly what actions are happening. From there you might either need to look into the GitSimBaseCommand to find the inherited method, or just look lower down the same module if the method was overridden.

BUT, I am curious what kind of other structures might be appropriate, while taking into account some of my design preferences regarding separation of scene and command, and also using inheritance to encapsulate the functionality for methods that are common to all (or many) commands.

Can you provide a sample structure of how Typer would adjust the structure of this codebase?

[paketb0te]

he only one I can think of is what we talked about before where the GitSim class passes itself into each Git subcommand instance, like self.command = GitSimLog(self).

Yeah I was referring to that - it's not really a circular reference I guess, bad naming on my part.

I'll try to hack something together to show what I think could be useful with Typer, might take a while though because I don't have much time the next days...

[initialcommit-io]

Cool, looking forward to seeing it! I do think that if we decide to do a structural refactoring of the codebase we should do it earlier as opposed to later, while the codebase is still pretty small.